nunit-core team mailing list archive

[Charlie Poole <charlie@xxxxxxxxx>]

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.


** Changed in: nunitv2/2.5
       Status: Triaged => Fix Committed

-- 
You received this bug notification because you are a member of NUnit
Developers, which is subscribed to NUnit V2.
https://bugs.launchpad.net/bugs/736062

Title:
  Deadlock when EventListener performs a Trace call + EventPump
  synchronisation

Status in NUnit Test Framework:
  Triaged
Status in NUnit V2 Test Framework:
  In Progress
Status in NUnit V2 2.5 series:
  Fix Committed
Status in NUnit V2 trunk series:
  In Progress

Bug description:
  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