Fixed a bug in the Transaction class that would skip on the AsyncEnded subscriber callbacks if a subscriber unsubscribed during the callback (either in the callback thread or asynchronously at the same time); wrote a unit test that highlights the bug

git-svn-id: file:///srv/devel/repo-conversion/nusu@125 d2e56fa2-650e-0410-a79f-9358c0239efd

Source/Tracking/Transaction.Test.cs

@@ -62,6 +62,46 @@ namespace Nuclex.Support.Tracking {
 
     #endregion // class TestWiatable
 
+    #region class UnsubscribingTransaction
+
+    /// <summary>Transaction that unsubscribes during an event callback</summary>
+    private class UnsubscribingTransaction : Transaction {
+
+      /// <summary>Initializes a new unsubscribing transaction</summary>
+      /// <param name="transactionToMonitor">
+      ///   Transaction whose AsyncEnded event will be monitored to trigger
+      ///   the this transaction unsubscribing from the event.
+      /// </param>
+      public UnsubscribingTransaction(Transaction transactionToMonitor) {
+        this.transactionToMonitor = transactionToMonitor;
+        this.monitoredTransactionEndedDelegate = new EventHandler(
+          monitoredTransactionEnded
+        );
+
+        this.transactionToMonitor.AsyncEnded += this.monitoredTransactionEndedDelegate;
+      }
+
+      /// <summary>Called when the monitored transaction has ended</summary>
+      /// <param name="sender">Monitored transaction that has ended</param>
+      /// <param name="arguments">Not used</param>
+      private void monitoredTransactionEnded(object sender, EventArgs arguments) {
+        this.transactionToMonitor.AsyncEnded -= this.monitoredTransactionEndedDelegate;
+      }
+
+      /// <summary>Transitions the transaction into the ended state</summary>
+      public void End() {
+        OnAsyncEnded();
+      }
+
+      /// <summary>Transaction whose ending in being monitored</summary>
+      private Transaction transactionToMonitor;
+      /// <summary>Delegate to the monitoredTransactionEnded() method</summary>
+      private EventHandler monitoredTransactionEndedDelegate;
+
+    }
+
+    #endregion // class TestWiatable
+
     /// <summary>Initialization routine executed before each test is run</summary>
     [SetUp]
     public void Setup() {
@@ -167,6 +207,23 @@ namespace Nuclex.Support.Tracking {
       Assert.IsTrue(test.Wait(TimeSpan.Zero));
     }
 
+    [Test]
+    public void TestUnsubscribeInEndedCallback() {
+      TestTransaction monitored = new TestTransaction();
+      UnsubscribingTransaction test = new UnsubscribingTransaction(monitored);
+
+      ITransactionSubscriber mockedSubscriber = mockSubscriber(monitored);
+      
+      try {
+        Expect.Once.On(mockedSubscriber).Method("Ended").WithAnyArguments();
+        monitored.End();
+        this.mockery.VerifyAllExpectationsHaveBeenMet();
+      }
+      finally {
+        test.End();
+      }
+    }
+
     /// <summary>Mocks a subscriber for the events of a transaction</summary>
     /// <param name="transaction">Transaction to mock an event subscriber for</param>
     /// <returns>The mocked event subscriber</returns>

Source/Tracking/Transaction.cs

@@ -103,20 +103,24 @@ namespace Nuclex.Support.Tracking {
       }
       remove {
 
-        lock(this) {
+        if(!this.ended) {
+          lock(this) {
+            if(!this.ended) {
 
-          // Only try to remove the event handler if the subscriber list was created,
+              // Only try to remove the event handler if the subscriber list was created,
-          // otherwise, we can be sure that no actual subscribers exist.
+              // otherwise, we can be sure that no actual subscribers exist.
-          if(!ReferenceEquals(this.endedEventSubscribers, null)) {
+              if(!ReferenceEquals(this.endedEventSubscribers, null)) {
-            int eventHandlerIndex = this.endedEventSubscribers.IndexOf(value);
+                int eventHandlerIndex = this.endedEventSubscribers.IndexOf(value);
+
+                // Unsubscribing a non-subscribed delegate from an event is allowed and
+                // should not throw an exception.
+                if(eventHandlerIndex != -1) {
+                  this.endedEventSubscribers.RemoveAt(eventHandlerIndex);
+                }
+              }
 
-            // Unsubscribing a non-subscribed delegate from an event is allowed and should
-            // not throw an exception.
-            if(eventHandlerIndex != -1) {
-              this.endedEventSubscribers.RemoveAt(eventHandlerIndex);
             }
           }
-
         }
 
       }
@@ -202,9 +206,9 @@ namespace Nuclex.Support.Tracking {
       lock(this) {
 
         // No double lock here, this is an exception that indicates an implementation
-        // error that will not be triggered under normal circumstances. We don't want
+        // error that will not be triggered under normal circumstances. We don't need
         // to waste any effort optimizing the speed at which an implementation fault
-        // will be noticed.
+        // will be reported ;-)
         if(this.ended)
           throw new InvalidOperationException("The transaction has already been ended");
 
@@ -214,21 +218,20 @@ namespace Nuclex.Support.Tracking {
         // the event after we just saw it being null, it would be created in an already
         // set state due to the ended flag (see above) being set to true beforehand!
         // But since we've got a lock ready, we can even avoid that 1 in a million
-        // performance loss and prevent a second doneEvent from being created.
+        // performance loss and prevent the doneEvent from being signalled needlessly.
         if(this.doneEvent != null)
           this.doneEvent.Set();
 
       }
 
+      // Fire the ended events to all event subscribers. We can freely use the list
+      // without synchronization at this point on since once this.ended is set to true,
+      // the subscribers list will not be accessed any longer
       if(!ReferenceEquals(this.endedEventSubscribers, null)) {
-
-        // Fire the ended events to all event subscribers. We can freely use the list
-        // without synchronization at this point on since once this.ended is set to true,
-        // the subscribers list will not be accessed any longer
         for(int index = 0; index < this.endedEventSubscribers.Count; ++index) {
           this.endedEventSubscribers[index](this, EventArgs.Empty);
         }
-
+        this.endedEventSubscribers = null;
       }
 
     }