[Live-devel] correct way to use TaskScheduler::unscheduleDelayedTask?

[Helmut Grohne]

Thank you for the quick reply.

On Tue, Aug 23, 2016 at 05:14:04PM +0200, Ross Finlayson wrote:
> I’m not sure I understand the motivation for your question, because “unscheduleDelayedTask()” is a function that should be called only rarely.  Note that once a ‘schedule task’ has occurred (once), it will not occur again.  (If you want a ‘scheduled task’ to occur periodically, you have to explicitly re-call “scheduleDelayedTask()” in the task handler.)  You need to call “unscheduleDelayedTask()” ONLY IF you have a pending ‘scheduled task’ that has not yet occurred.

I understand all of the above. Still, even a rare operation needs to
have well-defined semantics.

> Furthermore, once a ‘scheduled task’ has occurred, its ‘TaskToken’ is no longer valid, and should not be used again.  Therefore, you MUST NOT call “unscheduleDelayedTask()” (or “rescheduleDelayedTask()”) on a ‘TaskToken’ after its scheduled task has occurred.

This very much sounds like what I called option 3. I caution that
updating the documentation is insufficient, because live555 does not
follow this rule itself.

Let me give a concrete example (there are more). The Medium destructor
unconditionally unschedules its fNextTask member. That member can be in
any of three states:
 * NULL (from the constructor[1] or from a previous
   unscheduleDelayedTask)
 * valid TaskToken (after scheduleDelayedTask)
 * expired TaskToken (after running the associated task)

In order for the destructor to be correct by your rule, fNextTask must
never hold an expired TaskToken.

Now let us look into the BasicUDPSink subclass. Its afterGettingFrame1
method assigns a valid TaskToken to fNextTask by calling
scheduleDelayedTask. However, the recorded TaskFunc
BasicUDPSink::sendNext does not update the fNextTask field, so it is now
expired (i.e. invalid). If the object is destructed in an inopportune
moment, the destructor runs unscheduleDelayedTask on the expired
TaskToken, which violates your rule.

This is just one example, you can recognize the very same pattern in
e.g. GenericMediaServer::ClientSession::livenessTimeoutTask,
MultiFramedRTPSink::sendNext and a pile of others.

I stress that this is not a theoretical problem. If one implements a
TaskScheduler (as the faq advises) in a way that relies on this rule,
one can reissue the same TaskToken after it has expired and thus arrive
at double freeing memory.

I also stress that this choice makes it impossible to implement a
TaskScheduler that actually makes unscheduleDelayedTask thread-safe. (I
understand that unscheduleDelayedTask is not meant to be thread-safe and
that the BasicTaskScheduler is not implemented in a way that makes it
thread-safe.) By moving to the (a bit counter-intuitive) option 2
(always call unscheduleDelayedTask), a thread-safe implementation would
become feasible.

Helmut

[1] Technically, the fact that NULL is a special TaskToken that may
    always be unscheduled is not documented either. live555 relies on
    this in even more places.