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

[Helmut Grohne]

On Fri, Aug 26, 2016 at 12:07:45PM +0200, Ross Finlayson wrote:
> Yes, you are correct here.  Fortunately, there is an easy fix (which I’ll add to the code in the near future): Set “fNext” to NULL at the start of the handler function (“sendNext()”).  Ditto for the other places in the code where the same thing is done.

That's exactly what my patch does. It just looks long, because there are
so many places that lack these NULL assignments. Did you read it?

> Are you actually seeing these ‘double frees’ in your own application?  If you are, then I’m a bit surprised, because nobody else has reported seeing these.  Yes, the problem that you note is real - but for most people, it apparently doesn’t happen (because apparently - in most cases - “Medium” objects do not get destroyed between the handling/scheduling of delayed tasks).  But if you’re actually seeing this happen, then it appears that your application is doing something unusual, which makes me worried, because of what you wrote next:

Yes, I am. I guess I am combining two unusual things. Instead of using
BasicTaskScheduler, I am using my own scheduler[1]. The other aspect is
that I am testing the teardown path for a rtsp server. Only the
combination runs into these double frees. In this scheduler, there is a
particular assertion for catching some of these double frees, which can
otherwise go unnoticed.

If you add an assert(entry != NULL); to DelayQueue::removeEntry, you
should be seeing the issue in the teardown path as well. Since an
expired (invalid) entry is removed from the queue, looking up such a
token should result in entry being NULL.

> I hope you have read the FAQ (that everyone was asked to read before posting to the mailing list :-) - specifically:
> 	http://live555.com/liveMedia/faq.html#threads

I did. See below.

> You absolutely MUST NOT try to access a “TaskScheduler” (or any other LIVE555 object) from more than one thread.  As explained in the FAQ, the LIVE555 code was designed to be run as a single-threaded event loop.  (It *is* possible, however, to run LIVE555 code from more than one thread, if each thread uses its own “UsageEnvironment” and “TaskScheduler” object - again, as explained in the FAQ.)

Well, that FAQ entry is slightly wrong here. It really should say
BasicTaskScheduler instead of TaskScheduler. If a downstream of
liveMedia implements a subclass of TaskScheduler in a thread-safe way
(and that includes never accessing other live555 objects from other
threads), then accessing such a TaskScheduler subclass from another
thread is indeed safe.

In fact, even the FAQ advises that accessing TaskScheduler::triggerEvent
is supposed to be thread-safe (even though
BasicTaskScheduler::triggerEvent is not thread-safe due to the missing
memory ordering and atomicity primitives). As you can see, the FAQ
contradicts itself and only a look at the code reveals the truth.

So given the above, implementing TaskScheduler-subclass that has a
thread-safe triggerEvent method sounds like a good idea, doesn't it?

There is one aspect where thread-safety is relevant to the double frees.
By setting the watch variable from another thread (which is supposedly
supported according to the FAQ), doEventLoop can be interrupted in
unexpected states. Thus, the issue may indeed only be observable with
another thread setting the watch variable.

Helmut

[1] Once it works practically, we can consider including it into
    liveMedia, but I caution that it really is Linux-only.