Deadlock when EventListener performs a Trace call + EventPump synchronisation

[PA]

This is a sequel to the "Deadlock when EventListener performs a Trace call" message on NUnit Developer List.

http://groups.google.com/group/nunit-developer/browse_thread/thread/9cb94a7326c2c691

The basic problem is:

My addin installs an EventListener which can perform some Trace calls. This may lead to a deadlock, because:

(1) NUnit's EventPump.PumpThreadProc holds a lock on the EventQueue instance while sending events.

(2) The Trace code holds a lock while writing a trace message.

From the 3 solutions outined in the developer list message I chose the third, because EventPump.PumpThreadProc holding a lock while sending the events is a general design smell 8-).

There is a patch based on NUnit 2.5.9.10348 attached for EventPump.cs, EventQueue.cs and EventQueueTests.cs, which I kindly request to be considered for NUnit core. I compiled and tested it under Windows, MS .NET frameworks 1.1, 2.0 and 4.0.

Following modifications are included:

(1) EventPump.PumpThreadProc no longer holds a lock while sending the events. Instead, the locking is done internally in EventQueue.Enqueue and EventQueue.Dequeue.

(2) EventQueue.Enqueue calls Thread.Sleep(0) at its end, and the priority of the EventPump Thread is set to Highest, to help the EventPump to keep up with incoming events.

Item (1) prevents a deadlock, but in spite of item (2) increases the time gap from enqueuing an event to its delivery, as a side effect. This exhibited a synchronisation bug in my addin when the RunStarted event came too late. Instead of fixing it in my addin, I chose to add some synchronisation to the EventPump, as other NUnit users also reported problems with the asynchronous event delivery. In detail:

(3) Events got a new property IsSynchronous.

(4) If IsSynchronous=true, EventQueue.Enqueue blocks on an AutoResetEvent after adding an Event to the queue. EventPump.PumpThread releases the thread waiting in Enqueue by setting the AutoResetEvent after it has delivered the event.

There is just one AutoResetEvent instance for the EventQueue, which works as long as there is at most one thread enqueuing synchronous events. If there were concurrent threads producing synchronous events, there would have to be one AutoResetEvent instance per synchronous Event.

(5) Currently, IsSynchronous=true for RunStartedEvent, SuiteStartedEvent and TestStartedEvent, because for these, synchronous delivery is likely to be crucial. Now, an addin can rely on receiving these events before the Run/Suite/Test actually starts.

Optionally, one could make the *Finished events synchronous, too. OutputEvent may not be synchronous, because this would provoke deadlocks, again. UnhandledExceptionEvent may not be synchronous, because only one thread may produce synchronous events.

(6) To avoid having EventQueue implement IDisposable, the EventPump injects the AutoResetEvent instance into the EventQueue and is also responsible for disposing it again. See also the remark below.

(7) Unrelated to the main issue, another improvement in the Event classes: If a call to EventListener.RunFinished or EventListener.UnhandledException throws a SerializationException, because the exception parameter can not be serialized over an AppDomain boundary, the exception information is packed into a standard Exception instance, and the EventListener call is tried again.

Some more remarks:

I tried to minimize changes to the EventQueue: If SetWaitHandleForSynchronizedEvents is not called, the EventQueue works basically as before. So, other developers may still use the EventQueue for their own purposes without bothering about IsSynchronized etc., and without need to Dispose an EventQueue.

As an alternative, one could clean up the design further:
+ Encapsulate the AutoResetEvent instance completely in the EventQueue.
+ As a consequence, the EventQueue would have to dispose the AutoResetEvent and to implement IDisposable itself.
+ Currently, a QueuingEventListener creates and "owns" its EventQueue instance. When EventQueue becomes IDisposable, it would be better if the EventPump creates, owns and disposes the EventQueue instead.

Charlie, if you prefer this design alternative, I'd volunteer to implement it, too 8-).

Best regards,
Peter

[Charlie Poole (charlie.poole)]

This is a great patch - it fixes the deadlock issue and also cleans up a few other problems in passing, along with adding a bunch of new tests. I have applied it to the 2.5 branch and will next apply it to trunk after checking for any conflicting changes.