Padraig O'Briain wrote: > The call to do_next_check does the following: > > It determines whether a check for updates should be done (the call to > is_check_required). > If it is, a call to check_for_updates is done. If not > schedule_next_check_for_updates is called to schedule the next call to > do_next_check. > > I believe that the current implementation does the right thing when the > timeout period is increased or decreased. > > If the timeout period is increased we wake up when the previous timeout > period would have expired and schedule the call to do_next_check based > on the current timeout period. > > If the timeout period has decreased we determine whether we should > immediately call check_for_updates and if not we schedule the call to > do_next-check based on the current timeout period. > > Padraig > > Padraig > > On 12/05/08 21:04, Brock Pytlik wrote: >> 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. Hi, Given that info and the previous conversation I've commited the changes on Padraig's behalf: changeset: 766:300c8f10f13d
best Michal >> 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 _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
