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. I disagree that this is the correct behavior. Suppose a refresh has just happened and then I set the refresh period to be one week. The next refresh will be scheduled for one week from now. Then I change it to one day, and then immediately back to a week. The next refresh will now be scheduled tomorrow, not next week because it dropped to tomorrow when set to one day, but wasn't pushed back out to the one week mark.
That said, at this point, this is at worst, a small irritant to the user. While I'd like to see this more closely match my (and by hypothetical extension the general user's) expectations on behavior, I don't think it's worth holding this wad up anymore. If it goes back as is, I'll file a low priority bug to track the issue and let you decide if/when to handle. > > 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. > You're correct, I hadn't quite gotten the full flow through all the code in my head and had missed that __do_next_check wasn't immediately doing the check but instead scheduling the next check (for now if necessary). Brock > 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. 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
