You are viewing a plain text version of this content. The canonical link for it is here.
Posted to users@qpid.apache.org by Kim van der Riet <ki...@redhat.com> on 2018/07/19 15:11:54 UTC
[qpid-cpp] Inconsistent handling of qpid::sys::TimerTask
setUpNextFire() and restart() methods
The TimerTask class has two methods which change and/or set the time
when the timer will fire. Both methods assume a timer period has been
set when the task was originally created.
setUpNextFire() is called *after* the timer has already fired, and
allows the task to be fired again for the same period as originally used.
restart() is called *before* the timer has fired, and resets the timer
so that the original period is restarted from the moment of the call,
effectively delaying the firing.
Currently, the setUpNextFire() method checks that the timer has in fact
fired, and that the period has been set before allowing the nextFireTime
to be set for a future time.
However, the restart() method makes no equivalent check, and allows the
nextFireTime to be moved forward irrespective of the state of the timer.
Surely this method needs to check that the timer has *not yet* fired
before allowing the nextFireTime to be moved forward? I am looking at a
multithreaded case right now where the timer is allowing a restart() to
occur *after* the timer has moved from state WAITING to state CALLING,
ie the timer has fired.
It seems to me that a check similar to that used in setUpNextFire()
should be present in restart(). The conditions under which it reports an
error if it should be illegally called is debatable, however.
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org
Re: [qpid-cpp] Inconsistent handling of qpid::sys::TimerTask
setUpNextFire() and restart() methods
Posted by Kim van der Riet <ki...@redhat.com>.
On 07/20/2018 10:28 AM, Gordon Sim wrote:
> I thought what you wanted was a third option - a delay() - which would
> postpone the task to the full timeout again, but only if it had not
> already run. If so, that seems reasonable to me and should be easy to
> add. I think adding is safer than modifying one of the existing behaviours.
I think this may be the right approach. Thank you for your thoughts and
feedback.
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org
Re: [qpid-cpp] Inconsistent handling of qpid::sys::TimerTask
setUpNextFire() and restart() methods
Posted by Andrew Stitcher <as...@apache.org>.
On Fri, 2018-07-20 at 11:27 -0400, Kim van der Riet wrote:
> ...
> I am using the timer in periodic mode, ie with period set to an
> interval
Thanks for describing what you are trying to do: As I mentioned earlier
your case best matches the one-shot mode not the periodic mode.
> ...
> The idea is as follows: If the timer fires after some journal
> activity
> (but not enough to fill the write buffer), the journal is flushed.
> Only
> when new journal activity occurs, setUpNextFire() is called to start
> the
> timer again.
I think this is incorrect usage: You should be using restart() to delay
timer firing whenever the journal gets flushed by the journal writes.
Once the one-shot is fired you need to create a new TimerTask for
further timed flushes.
I think that your code is overcomplicated for what it needs to do. The
effect you seek is to timeout after journal flushes, in this case you
only need to create a one-shot TimerTask (and recreate it whenever it
gets fired). Whenever the journal code flushes because it has enough
data it uses restart() to delay the timer firing.
If the timer fires then we know that the timer time has elapsed since
the last journal flush and we do a flush. In this case we need to
create a new TimerTask as the one-shot has fired and cannot be used
again.
Andrew
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org
Re: [qpid-cpp] Inconsistent handling of qpid::sys::TimerTask
setUpNextFire() and restart() methods
Posted by Kim van der Riet <ki...@redhat.com>.
On 07/20/2018 11:04 AM, Gordon Sim wrote:
> On 20/07/18 15:50, Andrew Stitcher wrote:
>> On Fri, 2018-07-20 at 10:47 -0400, Andrew Stitcher wrote:
>>> On Fri, 2018-07-20 at 15:44 +0100, Gordon Sim wrote:
>>>> ...
>>>> I believe Kim is using restart() but is debugging some issue where
>>>> the
>>>> task is postponed after it has already fired, which he does not
>>>> want
>>>> (not sure why not). Is that right Kim?
>>>
>>> In that case I think you need to create a new Timer instance.
>>>
>>> IIRC once a one shot timer has elapsed you can't restart it any more
>>> -
>>> I may have got that wrong though as I didn't check the code
>>> thoroughly.
>>
>> To (try to) clarify further: The Timwer class can be used in two
>> different ways (perhaps these should have actually been different
>> classes, but here we are!): A periodic mode and a one shot mode.
>> restart() is used for the one-shot mode to delay firing;
>> setupNextFiring() is used in the periodic mode to set up the next
>> firing. I don't think you can sensibly use both for the same timer.
>
> That is indeed illuminating!
>
> And I think I probably mis-stated Kim's issue. Perhaps it is that the
> task is not being delayed because it has already fired?
>
I am using the timer in periodic mode, ie with period set to an interval
that is used to repeatedly set up a new firing. I have created a
subclass of qpid::sys::TimerTask, but the state of the TimerTask is
private, and the subclass has no visibility of the firing state of the
TimerTask. The subclassed TimerTask
(qpid::linearstore::journal::InactivityFireEvent) keeps its own state to
determine how to interact with the parent class.
The idea is as follows: If the timer fires after some journal activity
(but not enough to fill the write buffer), the journal is flushed. Only
when new journal activity occurs, setUpNextFire() is called to start the
timer again.
If the journal buffer fills before the timer fires, then if flushes on
its own. In this case, if the next journal activity starts before the
timer fires, will restart the timer. If the timer fires after a flush
but before further activity, the fire does nothing and returns.
The problem I am seeing is described in some detail at
https://bugzilla.redhat.com/show_bug.cgi?id=1599778#c12. It is a
multi-threaded issue in which if the following occur almost at the same
time on separate threads, then the timer can be placed in an errored
state: the timer fires, a flush occurs directly from the journal, and
journal write activity occurs that attempts to reset the timer.
My contention is that there is no reasonable way to avoid this issue
using TimerTask::restart() without a check that the timer has not yet
fired. It is because this *can* occur that an error condition is
created, which in turn results in the error messages seen in the above BZ.
Perhaps adding a method which includes this check for restarting the
timer would be the simplest solution.
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org
Re: [qpid-cpp] Inconsistent handling of qpid::sys::TimerTask
setUpNextFire() and restart() methods
Posted by Andrew Stitcher <as...@apache.org>.
On Fri, 2018-07-20 at 12:52 -0400, Andrew Stitcher wrote:
> I've now looked at the actual code!
>
> Things are a bit more complicated than I remembered:
I forgot one semi-important difference between setupNextFireTime() and
restart().
setupNextFireTime() is intended to be used for periodic tasks where the
period matters, so it sets up the firings to be equally spaced in time:
so it sets up the next fire time to be the previous_fire_time + period
(*not* now + period). This will lead to the average period tending to
the specified period irrespective of how long it takes to service the
timer firing.
However restart() just sets the next fire to now + period. So is not
useful if you want to keep a fairly strict period. This is why you can
use restart() after firing, you may just want another callback after
the period not at the correct time for a periodic timer.
So although at first glance they may do the same thing after the timer
has fired, this is not actually the case. As I have stated a number of
times now I believe that in the journal case only restart() should be
used.
Andrew
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org
Re: [qpid-cpp] Inconsistent handling of qpid::sys::TimerTask
setUpNextFire() and restart() methods
Posted by Andrew Stitcher <as...@apache.org>.
I've now looked at the actual code!
Things are a bit more complicated than I remembered:
TimerTasks can have two different types of time to trigger them, a
target absolute time ("at 3pm on 3 Jan 2016 UTC") and a relative time
("in 300ms"). You can only create one or the other of these TimerTask
(each has one constructor).
Both restart() and setupNextFireTime() are only useful in the relative
time case.
In fact I don't think the absolute case is ever used in Qpid - but if
it were neither of these methods could be used and the task can only
either fire or be cancelled.
In fact although restart() is used for the one shot case it cas
actually be used to restart a timer after it has fired as well - I was
wrong there.
What you can't do is mix using setupNExtfireTime() and restart() in the
same timer use (or at least not without pain).
The bit that I had confused with firing is that once the timer is
cancelled via cancel() the only thing you can do is destroy it and
create a new timer for use.
In Kim's case I think he should only be using restart().
Andrew
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org
Re: [qpid-cpp] Inconsistent handling of qpid::sys::TimerTask
setUpNextFire() and restart() methods
Posted by Gordon Sim <gs...@redhat.com>.
On 20/07/18 15:50, Andrew Stitcher wrote:
> On Fri, 2018-07-20 at 10:47 -0400, Andrew Stitcher wrote:
>> On Fri, 2018-07-20 at 15:44 +0100, Gordon Sim wrote:
>>> ...
>>> I believe Kim is using restart() but is debugging some issue where
>>> the
>>> task is postponed after it has already fired, which he does not
>>> want
>>> (not sure why not). Is that right Kim?
>>
>> In that case I think you need to create a new Timer instance.
>>
>> IIRC once a one shot timer has elapsed you can't restart it any more
>> -
>> I may have got that wrong though as I didn't check the code
>> thoroughly.
>
> To (try to) clarify further: The Timwer class can be used in two
> different ways (perhaps these should have actually been different
> classes, but here we are!): A periodic mode and a one shot mode.
> restart() is used for the one-shot mode to delay firing;
> setupNextFiring() is used in the periodic mode to set up the next
> firing. I don't think you can sensibly use both for the same timer.
That is indeed illuminating!
And I think I probably mis-stated Kim's issue. Perhaps it is that the
task is not being delayed because it has already fired?
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org
Re: [qpid-cpp] Inconsistent handling of qpid::sys::TimerTask
setUpNextFire() and restart() methods
Posted by Andrew Stitcher <as...@apache.org>.
On Fri, 2018-07-20 at 10:47 -0400, Andrew Stitcher wrote:
> On Fri, 2018-07-20 at 15:44 +0100, Gordon Sim wrote:
> > ...
> > I believe Kim is using restart() but is debugging some issue where
> > the
> > task is postponed after it has already fired, which he does not
> > want
> > (not sure why not). Is that right Kim?
>
> In that case I think you need to create a new Timer instance.
>
> IIRC once a one shot timer has elapsed you can't restart it any more
> -
> I may have got that wrong though as I didn't check the code
> thoroughly.
To (try to) clarify further: The Timwer class can be used in two
different ways (perhaps these should have actually been different
classes, but here we are!): A periodic mode and a one shot mode.
restart() is used for the one-shot mode to delay firing;
setupNextFiring() is used in the periodic mode to set up the next
firing. I don't think you can sensibly use both for the same timer.
Caveat, this is from memory, and may be wrong in light of the actual
code!
Andrew
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org
Re: [qpid-cpp] Inconsistent handling of qpid::sys::TimerTask
setUpNextFire() and restart() methods
Posted by Andrew Stitcher <as...@apache.org>.
On Fri, 2018-07-20 at 15:44 +0100, Gordon Sim wrote:
> ...
> I believe Kim is using restart() but is debugging some issue where
> the
> task is postponed after it has already fired, which he does not want
> (not sure why not). Is that right Kim?
In that case I think you need to create a new Timer instance.
IIRC once a one shot timer has elapsed you can't restart it any more -
I may have got that wrong though as I didn't check the code thoroughly.
Andrew
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org
Re: [qpid-cpp] Inconsistent handling of qpid::sys::TimerTask
setUpNextFire() and restart() methods
Posted by Gordon Sim <gs...@redhat.com>.
On 20/07/18 15:42, Andrew Stitcher wrote:
> I think I can help shed some light on the original intention of these
> two methods:
>
> Essentially they are intended for different purposes: (As Gordon says)
> setUpNextFire() is intended only for use with periodic timers and is
> intended to be used to set up the next firing *after the time has just
> fired*; restart() is intended to be used to delay firing a timer and so
> is used *before* a timer has elapsed - specifically restart() is used
> where you have a time out that needs to be extended because the
> something happened.
>
> For example restart() is useed in processing idle timeout in qpid. Such
> that a timer is set for the timeout and every time a frame is received
> the timer is restarted, so that the timer is not fired unless the
> timeout occurs from the last frame received.
>
> I think in the case of the journal flush restart is the likely
> semantics you want - you want to have an idle timer timeout which is
> extended (and hence not fired) every time the journal gets written. If
> the timeout elapses after the last journal write the time will fire.
I believe Kim is using restart() but is debugging some issue where the
task is postponed after it has already fired, which he does not want
(not sure why not). Is that right Kim?
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org
Re: [qpid-cpp] Inconsistent handling of qpid::sys::TimerTask
setUpNextFire() and restart() methods
Posted by Andrew Stitcher <as...@apache.org>.
I think I can help shed some light on the original intention of these
two methods:
Essentially they are intended for different purposes: (As Gordon says)
setUpNextFire() is intended only for use with periodic timers and is
intended to be used to set up the next firing *after the time has just
fired*; restart() is intended to be used to delay firing a timer and so
is used *before* a timer has elapsed - specifically restart() is used
where you have a time out that needs to be extended because the
something happened.
For example restart() is useed in processing idle timeout in qpid. Such
that a timer is set for the timeout and every time a frame is received
the timer is restarted, so that the timer is not fired unless the
timeout occurs from the last frame received.
I think in the case of the journal flush restart is the likely
semantics you want - you want to have an idle timer timeout which is
extended (and hence not fired) every time the journal gets written. If
the timeout elapses after the last journal write the time will fire.
Hope this helps.
Andrew
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org
Re: [qpid-cpp] Inconsistent handling of qpid::sys::TimerTask
setUpNextFire() and restart() methods
Posted by Gordon Sim <gs...@redhat.com>.
On 20/07/18 15:18, Kim van der Riet wrote:
>
>
> On 07/20/2018 10:00 AM, Gordon Sim wrote:
>> On 20/07/18 14:50, Kim van der Riet wrote:
>>> I agree that this is ambiguous. However, if the intent is to allow
>>> the timer to have period added for any state, why do we need
>>> setUpNextFire()? This idea seems logically wrong.
>>
>> I believe setUpNextFire() was intended to be used by the Task subclass
>> itself in order to repeat an action (i.e. to have a periodic task).
>>
> Agreed, but since I need to track the state of the timer using external
> logic / state anyway (ie only call setUpNextFire() after the fire has
> happened), why can't I use restart for that instead? It does exactly
> the same thing, just without the check that the timer has fired.
Not really sure what you are asking.
There is setUpNextFire() which ensures that it is called after the task
has fired and only if it was periodic (i.e. had a duration). Then there
is restart which just causes the timer to 'restart' i.e. to fire after
the desired interval starting from now has elapsed. That has no check of
current state or whether task has already fired.
I thought what you wanted was a third option - a delay() - which would
postpone the task to the full timeout again, but only if it had not
already run. If so, that seems reasonable to me and should be easy to
add. I think adding is safer than modifying one of the existing behaviours.
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org
Re: [qpid-cpp] Inconsistent handling of qpid::sys::TimerTask
setUpNextFire() and restart() methods
Posted by Kim van der Riet <ki...@redhat.com>.
On 07/20/2018 10:00 AM, Gordon Sim wrote:
> On 20/07/18 14:50, Kim van der Riet wrote:
>> I agree that this is ambiguous. However, if the intent is to allow the
>> timer to have period added for any state, why do we need
>> setUpNextFire()? This idea seems logically wrong.
>
> I believe setUpNextFire() was intended to be used by the Task subclass
> itself in order to repeat an action (i.e. to have a periodic task).
>
Agreed, but since I need to track the state of the timer using external
logic / state anyway (ie only call setUpNextFire() after the fire has
happened), why can't I use restart for that instead? It does exactly
the same thing, just without the check that the timer has fired.
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org
Re: [qpid-cpp] Inconsistent handling of qpid::sys::TimerTask
setUpNextFire() and restart() methods
Posted by Gordon Sim <gs...@redhat.com>.
On 20/07/18 14:50, Kim van der Riet wrote:
> I agree that this is ambiguous. However, if the intent is to allow the
> timer to have period added for any state, why do we need
> setUpNextFire()? This idea seems logically wrong.
I believe setUpNextFire() was intended to be used by the Task subclass
itself in order to repeat an action (i.e. to have a periodic task).
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org
Re: [qpid-cpp] Inconsistent handling of qpid::sys::TimerTask
setUpNextFire() and restart() methods
Posted by Kim van der Riet <ki...@redhat.com>.
I agree that this is ambiguous. However, if the intent is to allow the
timer to have period added for any state, why do we need
setUpNextFire()? This idea seems logically wrong.
On 07/19/2018 03:32 PM, Gordon Sim wrote:
> The doxygen comment for the method states:
>
> This can be called either with the TimerTask already added to
> a Timer or after the task has been triggered. It has the effect
> of delaying the task triggering by the initially specified
> duration.
>
> The first sentence suggests it is intentional that it will 'restart' the
> timer even if already fired (or firing). (The second sentence admittedly
> muddies things a little, since you can't really delay something that has
> already happened).
>
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org
Re: [qpid-cpp] Inconsistent handling of qpid::sys::TimerTask
setUpNextFire() and restart() methods
Posted by Gordon Sim <gs...@redhat.com>.
On 19/07/18 20:28, Gordon Sim wrote:
> On 19/07/18 16:11, Kim van der Riet wrote:
>> The TimerTask class has two methods which change and/or set the time
>> when the timer will fire. Both methods assume a timer period has been
>> set when the task was originally created.
>>
>> setUpNextFire() is called *after* the timer has already fired, and
>> allows the task to be fired again for the same period as originally used.
>>
>> restart() is called *before* the timer has fired, and resets the timer
>> so that the original period is restarted from the moment of the call,
>> effectively delaying the firing.
>>
>> Currently, the setUpNextFire() method checks that the timer has in
>> fact fired, and that the period has been set before allowing the
>> nextFireTime to be set for a future time.
>>
>> However, the restart() method makes no equivalent check, and allows
>> the nextFireTime to be moved forward irrespective of the state of the
>> timer. Surely this method needs to check that the timer has *not yet*
>> fired before allowing the nextFireTime to be moved forward? I am
>> looking at a multithreaded case right now where the timer is allowing
>> a restart() to occur *after* the timer has moved from state WAITING to
>> state CALLING, ie the timer has fired.
>>
>> It seems to me that a check similar to that used in setUpNextFire()
>> should be present in restart(). The conditions under which it reports
>> an error if it should be illegally called is debatable, however.
>
> Not sure what the intention was, but perhaps the simplest solution would
> be to add another method that does what you want, i.e. only delays if
> timer has not fired, and returns a boolean indicating whether it delayed
> or not. That way you can get the semantics you want but the existing
> semantics aren't broken incase there is some subtle case that expects to
> be allowed to restart the timer even if it has just fired.
The doxygen comment for the method states:
This can be called either with the TimerTask already added to
a Timer or after the task has been triggered. It has the effect
of delaying the task triggering by the initially specified
duration.
The first sentence suggests it is intentional that it will 'restart' the
timer even if already fired (or firing). (The second sentence admittedly
muddies things a little, since you can't really delay something that has
already happened).
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org
Re: [qpid-cpp] Inconsistent handling of qpid::sys::TimerTask
setUpNextFire() and restart() methods
Posted by Gordon Sim <gs...@redhat.com>.
On 19/07/18 16:11, Kim van der Riet wrote:
> The TimerTask class has two methods which change and/or set the time
> when the timer will fire. Both methods assume a timer period has been
> set when the task was originally created.
>
> setUpNextFire() is called *after* the timer has already fired, and
> allows the task to be fired again for the same period as originally used.
>
> restart() is called *before* the timer has fired, and resets the timer
> so that the original period is restarted from the moment of the call,
> effectively delaying the firing.
>
> Currently, the setUpNextFire() method checks that the timer has in fact
> fired, and that the period has been set before allowing the nextFireTime
> to be set for a future time.
>
> However, the restart() method makes no equivalent check, and allows the
> nextFireTime to be moved forward irrespective of the state of the timer.
> Surely this method needs to check that the timer has *not yet* fired
> before allowing the nextFireTime to be moved forward? I am looking at a
> multithreaded case right now where the timer is allowing a restart() to
> occur *after* the timer has moved from state WAITING to state CALLING,
> ie the timer has fired.
>
> It seems to me that a check similar to that used in setUpNextFire()
> should be present in restart(). The conditions under which it reports an
> error if it should be illegally called is debatable, however.
Not sure what the intention was, but perhaps the simplest solution would
be to add another method that does what you want, i.e. only delays if
timer has not fired, and returns a boolean indicating whether it delayed
or not. That way you can get the semantics you want but the existing
semantics aren't broken incase there is some subtle case that expects to
be allowed to restart the timer even if it has just fired.
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org