Padraig O'Briain wrote: > I have made changes to address Brock's concerns. > > Webrev is http://cr.opensolaris.org/~padraig/ips-5188-v5/ > > MAXIMUM_WAIT no longer exists. > > When notification of change of refresh period is received we check > whether the new period is shorter than the old period and if so we > remove the scheduled call to do_next_check and immediately call > do_next_check. This is good enough for me. What I think should really be done is instead of immediately calling do_next_check, it should be scheduled at the appropriate time into the future.
For example, I'd do something like: store the time when a call is added to timeout_add, then when the user changes their refresh period, it takes the old time, adds the new delta to it, and subtracts the current time. If that number (call it time_to_next_call) is <=0, it does a call to do_next_check immediately, otherwise it does a timeout_add with time_to_next_call as the delay. This has the advantage of providing the expected (at least to me) behavior of moving the next check out further if I raise the timeout as well as moving it closer if I lower the timeout. Whichever you choose to do is fine with me, Brock > > Padraig > > On 12/04/08 20:00, Brock Pytlik wrote: >> Padraig O'Briain wrote: >>> [snip] >>> >>> Should self.reference_number be self.refresh_number? >> Yes >>> >>> If we do not use MAXIMUM_WAIT what time interval is used in call to >>> gobject_idle_add()? >> Do you mean timeout_add? (As far as I can tell, idle_add doesn't take >> an interval.) I would suggest using an interval so that >> is_check_required is called at the appropriate time (whatever the >> user has set their refresh period to). >> >> In short, I would get rid of schedule_next_check_for_checks because >> it's no longer needed because you're getting notification from gconf >> now. >> >>> >>> There should not be a need for locks as everything is executed in >>> one thread. >> My mistake then. So when a function is executed because its >> timeout_add timer has expired, that isn't spun into a separate >> thread? That does indeed make things simpler. >> >> I've also discovered that there is a way to remove things added via >> timeout_add from the queue. >> >> So, given this info, let me propose option 2: >> >> Where you do gobject.timeout_add(foo, self.do_next_check), store the >> source_id it returns and the absolute time that it will expire. [1] >> When gconf notifies that the refresh period has changed, check >> whether the new refresh period means that self.do_next_check should >> be called before it's currently scheduled to be. If so, remove the >> current function from the queue (using the source id previously >> stored), and add the new call to self.do_next_checks with the >> appropriate timeout. >> >> Brock >> >> [1] >> http://www.pygtk.org/pygtk2tutorial/ch-TimeoutsIOAndIdleFunctions.html#sec-Timeouts >> >> >> >>> >>> Padraig >>> >>>> The comments are better than nothing. I just don't understand why >>>> we wouldn't do it right in the first place. >>>> Because I've now hit the put up or shut up point personally, I'll >>>> add this: >>>> >>>> My first pass solution to this would be to add a >>>> self.refresh_number to the UMN class. When I added the next call to >>>> the timeout_add loop, I'd increment the self.refresh_number by 1, >>>> and pass it as an argument to the function. When the function is >>>> first executed, it checks to see whether self.reference_number >>>> matches the argument it was passed. If it does, it proceeds >>>> happily. If the argument is greater than self.reference_number, it >>>> fails an assert (because I can't see how this should ever happen), >>>> if the argument is less than self.reference_number, then it just >>>> returns False and exits. >>>> >>>> I'm sure there are details to be worked out with that >>>> implementation, a lock around self.refresh_number is probably >>>> necessary for example, but I don't see why those details would >>>> stand in the way of writing the code so that the maximum_wait isn't >>>> an issue. >>>> >>>> Also, having to manually sync MAXIMUM_WAIT and the other various >>>> intervals seems like a recipe for bugs as well. >>>> >>>> >> _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
