[Live-devel] triggerEvent() race condition

[Jeff Shanab]

I have used boost atomic or just a boost mutex with a scoped_lock when I
needed to protect, It implements cross platform the best avail on a
platform.




On Tue, Mar 17, 2015 at 9:07 AM, Robert Smith <smith at cognitec.com> wrote:

>  Ok, I will implement my own TaskScheduler class, but operations that
> read-modify-write memory are not atomic with multiple processors/cores. I
> guess the large portion of users these days will be using multicore x86
> processors.
>
> I asked this question on stackoverflow and the responses support the
> research I have done myself.
>
>
> http://stackoverflow.com/questions/29099094/is-the-c-operator-atomic-with-a-multicore-processor
>
> My updated  application with a protected triggerEvent() method didn't cure
> the problem, one (out of six) of the streams stopped after an hour. I then
> modified BasicTaskScheduler to protect all reads/writes of the
> fTriggersAwaitingHandling and it has been running for over 24 hours now.
> Time will tell but it looks promising.
>
> If I'm to speculate why this hasn't been a problem before I would guess
> that the large majority of users are not using the triggers at runtime,
> e.g, they are using the file sources. And for those who are using a live
> source, if they only have one stream then the task schedulers' SingleStep()
> method is probably waiting in the select() call when the triggerEvent()
> method is called making it unlikely that they conflict.
>
> Also each thread in my application has it's own event trigger id.
>
> I'm not even suggesting that you change the implementation but at least be
> aware of the problem and document it for future users.
>
>
> On 03/16/2015 05:50 PM, Ross Finlayson wrote:
>
>   Having thought about it some more, I don't think it's enough to just
> protect calls to triggerEvent() because 'fTriggersAwaitingHandling' is
> still modified by BasicTaskScheduler::SingleStep().
>
>
>  The intention of the code that implements “BasicTaskScheduler” was that:
> 1/ The event loop code (which is always just a single thread, remember)
> does nothing with “fTriggersAwaitingHandling” except clear (using “&=~”) a
> single bit that was already set, and
> 2/ The other thread(s) that can trigger event(s) do nothing
> with “fTriggersAwaitingHandling” except set (using “|=“) bit(s) that were
> not already set.
>
>  If these two conditions are met, *and* if the ‘clear a bit’ and ‘set a
> bit’ operations are atomic, then the code will be thread-safe.
>
>  (If, OTOH, you’re worried that the ‘clear a bit’ and ‘set a bit’
> operations might not be atomic on your system, then you’d need to write
> your own “TaskScheduler” subclass instead.)
>
>  Looking at the code again just now, however, I realized that the event
> loop code does not always satisfy condition 1/.   Line 183 of
> “BasicTaskScheduler.cpp” should be changed from:
>  fTriggersAwaitingHandling = 0;
> to
>  fTriggersAwaitingHandling &=~ fLastUsedTriggerMask;
>
>  I have just released a new version (2015.03.16) of the code that fixes
> this.  You should upgrade to this version.
>
>
>  You should also make sure that your own code satisfies condition 2/.
> I.e., you should make sure that your code is never calling “triggerEvent()”
> more than once with the same ‘event trigger’ value, unless you’re sure that
> the first event (for that event trigger) has already been handled.  (In
> particular, you should never call “triggerEvent()” with the same ‘event
> trigger’ value from more than one thread.)
>
>  Ross Finlayson
> Live Networks, Inc.
> http://www.live555.com/
>
>
>
>
> _______________________________________________
> live-devel mailing listlive-devel at lists.live555.comhttp://lists.live555.com/mailman/listinfo/live-devel
>
>
>
> _______________________________________________
> live-devel mailing list
> live-devel at lists.live555.com
> http://lists.live555.com/mailman/listinfo/live-devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.live555.com/pipermail/live-devel/attachments/20150317/f2cb9327/attachment.html>