Fix #55: dispatch events by thread identity, not context-instance equality

src/OSPSuite.Utility/Events/EventPublisher.cs

@@ -10,13 +10,22 @@ namespace OSPSuite.Utility.Events
    public class EventPublisher : IEventPublisher
    {
       private readonly SynchronizationContext _context;
+      private readonly Thread _contextThread;
       private readonly IExceptionManager _exceptionManager;
       private readonly IList<WeakRef<IListener>> _listeners;
 
+      // Assumes the constructing thread is the one the context dispatches to. Use the explicit-thread
+      // overload when the context targets a different thread (e.g. one captured for a separate UI thread).
       public EventPublisher(SynchronizationContext context, IExceptionManager exceptionManager)
+         : this(context, Thread.CurrentThread, exceptionManager)
+      {
+      }
+
+      public EventPublisher(SynchronizationContext context, Thread contextThread, IExceptionManager exceptionManager)
       {
          _listeners = new List<WeakRef<IListener>>();
          _context = context;
+         _contextThread = contextThread;
          _exceptionManager = exceptionManager;
       }
 
@@ -33,11 +42,11 @@ public void PublishEvent<T>(T eventToPublish)
             listeners = _listeners.ToList();
          }
 
-         // When publishing from the thread that owns the context (typically the UI thread),
-         // dispatch synchronously with Send so handlers run inline and any state they touch is
-         // updated before PublishEvent returns. When publishing from another thread, use the
-         // non-blocking Post so the background thread is not blocked waiting on the UI thread.
-         var publishingFromContextThread = SynchronizationContext.Current == _context;
+         // Decide Send vs Post by thread identity, not by context instance: a thread can have more than
+         // one SynchronizationContext instance targeting it, so instance comparison can wrongly Post
+         // while already on the context thread. On the context thread, Send dispatches inline (handlers
+         // run before this returns); from any other thread, Post so the publishing thread is not blocked.
+         var publishingFromContextThread = ReferenceEquals(Thread.CurrentThread, _contextThread);
 
          // Determine if a Listener handles the message of type T by trying to cast it.
          // Dispatch happens outside the lock so handlers never run while the lock is held.

tests/OSPSuite.Utility.Tests/EventPublisherSpecs.cs

@@ -12,7 +12,7 @@ namespace OSPSuite.Utility.Tests
 {
    public abstract class concern_for_EventPublisher : ContextSpecification<IEventPublisher>
    {
-      private IExceptionManager _exceptionManager;
+      protected IExceptionManager _exceptionManager;
       protected RecordingSynchronizationContext _synchronizationContext;
 
       protected override void Context()
@@ -32,10 +32,10 @@ public override void LogException(Exception ex)
          internal Exception Exception { get; set; }
       }
 
-      // Runs the given action on a fresh thread whose current SynchronizationContext is the one
-      // supplied, so a test can control whether PublishEvent sees itself as running on the
-      // publisher's context thread (pass _synchronizationContext) or off it (pass null). Joining
-      // the thread before returning makes the recorded flags safe to read on the test thread.
+      // Runs the given action on a fresh thread (distinct from the one that constructed the publisher),
+      // optionally installing the given SynchronizationContext on it. Publishing from here exercises the
+      // off-context-thread path, because the publishing thread is not the publisher's context thread.
+      // Joining the thread before returning makes the recorded flags safe to read on the test thread.
       protected static void runOnThreadWithSynchronizationContext(SynchronizationContext context, Action action)
       {
          Exception thrown = null;
@@ -108,13 +108,12 @@ public void should_notify_all_listeners_of_the_event()
       }
    }
 
-   public class When_publishing_an_event_from_the_thread_that_owns_the_synchronization_context : concern_for_publishing_an_event
+   public class When_publishing_an_event_from_the_publisher_context_thread : concern_for_publishing_an_event
    {
       protected override void Because()
       {
-         // simulate publishing from the UI thread: the publishing thread's current context
-         // is the very context the publisher dispatches to
-         runOnThreadWithSynchronizationContext(_synchronizationContext, () => sut.PublishEvent(_eventToPublish));
+         // publish on the same thread that constructed the publisher - i.e. its context thread
+         sut.PublishEvent(_eventToPublish);
       }
 
       [Observation]
@@ -131,12 +130,48 @@ public void should_notify_all_listeners_of_the_event()
       }
    }
 
+   public class When_publishing_an_event_from_the_context_thread_while_a_different_synchronization_context_is_current : concern_for_publishing_an_event
+   {
+      private SynchronizationContext _originalContext;
+
+      protected override void Because()
+      {
+         // The live SynchronizationContext.Current is a different instance than the one the publisher
+         // captured, but we are still on the publisher's context thread. Dispatch must stay on the Send
+         // (synchronous) path: this is the regression where instance comparison wrongly chose Post while
+         // already on the UI thread (e.g. closing the app), deferring handlers until after teardown.
+         _originalContext = SynchronizationContext.Current;
+         SynchronizationContext.SetSynchronizationContext(new RecordingSynchronizationContext());
+         try
+         {
+            sut.PublishEvent(_eventToPublish);
+         }
+         finally
+         {
+            SynchronizationContext.SetSynchronizationContext(_originalContext);
+         }
+      }
+
+      [Observation]
+      public void should_still_dispatch_synchronously_through_the_blocking_send_path()
+      {
+         _synchronizationContext.SendWasCalled.ShouldBeTrue();
+         _synchronizationContext.PostWasCalled.ShouldBeFalse();
+      }
+
+      [Observation]
+      public void should_notify_all_listeners_of_the_event()
+      {
+         A.CallTo(() => _listener.Handle(_eventToPublish)).MustHaveHappened();
+      }
+   }
+
    public class When_publishing_an_event_from_a_thread_that_does_not_own_the_synchronization_context : concern_for_publishing_an_event
    {
       protected override void Because()
       {
-         // simulate publishing from a background thread: the publishing thread does not share
-         // the publisher's context
+         // publish from a different thread than the one that constructed the publisher (a background
+         // thread): the publishing thread is not the publisher's context thread
          runOnThreadWithSynchronizationContext(null, () => sut.PublishEvent(_eventToPublish));
       }
 
@@ -154,6 +189,77 @@ public void should_notify_all_listeners_of_the_event()
       }
    }
 
+   public class When_constructed_with_an_explicit_context_thread_and_publishing_from_that_thread : concern_for_EventPublisher
+   {
+      private object _eventToPublish;
+      private IListener<object> _listener;
+
+      protected override void Context()
+      {
+         base.Context();
+         // dispatch thread is explicitly the current (publishing) thread
+         sut = new EventPublisher(_synchronizationContext, Thread.CurrentThread, _exceptionManager);
+         _eventToPublish = A.Fake<object>();
+         _listener = A.Fake<IListener<object>>();
+         sut.AddListener(_listener);
+      }
+
+      protected override void Because()
+      {
+         sut.PublishEvent(_eventToPublish);
+      }
+
+      [Observation]
+      public void should_dispatch_synchronously_through_the_blocking_send_path()
+      {
+         _synchronizationContext.SendWasCalled.ShouldBeTrue();
+         _synchronizationContext.PostWasCalled.ShouldBeFalse();
+      }
+
+      [Observation]
+      public void should_notify_all_listeners_of_the_event()
+      {
+         A.CallTo(() => _listener.Handle(_eventToPublish)).MustHaveHappened();
+      }
+   }
+
+   public class When_constructed_with_an_explicit_context_thread_other_than_the_publishing_thread : concern_for_EventPublisher
+   {
+      private object _eventToPublish;
+      private IListener<object> _listener;
+
+      protected override void Context()
+      {
+         base.Context();
+         // an unstarted thread is never the current thread, so the publisher's context thread differs
+         // from whatever thread publishes below - mirrors a context captured for a separate UI thread
+         // (e.g. the splash screen) being published to from the main thread
+         var contextThread = new Thread(() => { });
+         sut = new EventPublisher(_synchronizationContext, contextThread, _exceptionManager);
+         _eventToPublish = A.Fake<object>();
+         _listener = A.Fake<IListener<object>>();
+         sut.AddListener(_listener);
+      }
+
+      protected override void Because()
+      {
+         sut.PublishEvent(_eventToPublish);
+      }
+
+      [Observation]
+      public void should_dispatch_through_the_non_blocking_post_path()
+      {
+         _synchronizationContext.PostWasCalled.ShouldBeTrue();
+         _synchronizationContext.SendWasCalled.ShouldBeFalse();
+      }
+
+      [Observation]
+      public void should_notify_all_listeners_of_the_event()
+      {
+         A.CallTo(() => _listener.Handle(_eventToPublish)).MustHaveHappened();
+      }
+   }
+
    public class When_asked_to_add_a_listener : concern_for_EventPublisher
    {
       private IListener<object> _listener;