Re: EventLoop.isCurrentThread() race condition

[Kris Van Hees <kris dot van dot hees at oracle dot com>]

Agreed, and thanks for pointing out that I did actually overlook one
call to updateTid() (from run() in EventLoop).  However, I still do not
really see why isCurrentThread() would record the tid if it hasn't been
set yet.  It seems to be an undesirable complication that can give rise
to a race condition where none should exist.  Some tests in frysk-core
are actually using runPending or runPolling in a multi-threaded context,
which is obviously incorrect given your explanation below, but instead
of it triggering a clear exception, it causes an intermittent failure.
Such indeterministic behaviour should be avoided I think, especially as
it contributes to intermittent testsuite failures, reducing the
effectiveness of tests.

Also note that the comments on e.g. the run() method of EventLoop state
that existing pending events are processed before the first poll.  But
(especially in view of existing code in classes derived from Request)
that again opens up the problem that isCurrentThread() may be called
prior to EventLoop.start(), i.e. before the tid is assigned the
"correct" value.  Prohibiting pending events prior to the cal to start()
might be one way to go for multi-threaded applications, but that seems
to be the wrong way to go I think.

The proposed patch (if tid is -1, isCurrentThread() wil return false)
solves the problem of being indeterministic, and acts appropriately (as
far as I can determine) in both single threaded and multi threaded apps:

1. single threaded: No call has been made to runPending or runPolling, so
   the event loop has not executed yet.  If a call to isCurrentThread()
   is used to determine on whether a request should be executed
   synchronously or be added as an event, an event will be added.  It
   will still executed within the context of the single thread, albeit
   not until runPending or runPolling is executed.
2. multi threaded: The event loop is not running yet, so there isn't
   really an event loop thread yet.  Adding the request as an event to
   be processed as soonas the event loop thread starts seems quite
   correct in that case.  Alternatively, executing the request in the
   currently executing thread would mean later requests execute in a
   different thread (actual event loop), which is probably not what is
   supposed to be supported anyway.

Finally, if it is important for a request to be able to determine
whether isCurrentThread() returns false as a result of the event loop
not having been started (either as a separate thread or because
runPending/runPolling was not called yet) vs being a result of the
currently executing thread not being the event loop thread, it is easy
enough to add a method to EventLoop to query for that very situation
(e.g. hasStarted() or something similar).

	Cheers,
	Kris

On Fri, Jun 22, 2007 at 11:09:52AM -0400, Andrew Cagney wrote:
> There are two ways to run the eventLoop:
> 
> a) in the main thread of a single threaded app, as the frysk.proc tests 
> and utilities all do
> b) in a separate thread, started early by the main thread by having it 
> call eventLoop.start() (the gui has a bug here, npremji was looking at 
> it, part of which is not extending Thread)
> 
> That logic detects when neither of those strict models are being 
> followed - and a deadlock will likely result (the signal goes to the 
> wrong thread, or there is no thread to wake up).
> 
> Andrew
> 
> 
> the code you refer
> Kris Van Hees wrote:
> >I was wondering whether anyone is actually depending on the side effect
> >of the isCurrentThread() method in frysk.event.EventLoop, in that it
> >assigns the tid of the current thread as the identifier for the event
> >loop thread if it wasn't assigned before.
> >
> >Reason for asking is described in bugzilla #4688.  Essentially, we end
> >up with a race condition on what thread ends up setting the tid variable
> >in the EventLoop class.  There are cases where the EventLoop is
> >instantiated (tid is -1 at that point), and then other threads are
> >started that may trigger operations in the core that (usually
> >indirectly) call isCurrentThread() to determine whether work can be done
> >in-line or needs to be placed on the queue.  If that call to
> >isCurrentThread() is reached before the actual event loop thread
> >executes either runPolling() or runPending(), the event loop will
> >register the wrong tid, causing an exception later on (in updateTid())
> >when runPolling() or runPending() is called (from the correct thread).
> >
> >The fix is of course to remove the side effect from isCurrentThread(),
> >but I'd rather not commit that until I know that there isn't someone
> >who actually depends on this behaviour since it has been in existance
> >since Apr 18, 2007.
> >
> >For reference, I ran all tests with the current side effect in place,
> >and then re-ran all tests again with the patch applied.  The results
> >were identical (modulo tests that intermittently fail due to the
> >updateTid() esception).
> >
> >	Cheers,
> >	Kris
> >  
>