From dcf47768e57284ddcd8f88128745664bc802b9dc Mon Sep 17 00:00:00 2001 From: Markus Ewald Date: Wed, 1 Apr 2009 20:32:22 +0000 Subject: [PATCH] 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 | 57 +++++++++++++++++++++++++++++ Source/Tracking/Transaction.cs | 39 +++++++++++--------- 2 files changed, 78 insertions(+), 18 deletions(-) diff --git a/Source/Tracking/Transaction.Test.cs b/Source/Tracking/Transaction.Test.cs index 46ce19a..6deb1b0 100644 --- a/Source/Tracking/Transaction.Test.cs +++ b/Source/Tracking/Transaction.Test.cs @@ -62,6 +62,46 @@ namespace Nuclex.Support.Tracking { #endregion // class TestWiatable + #region class UnsubscribingTransaction + + /// Transaction that unsubscribes during an event callback + private class UnsubscribingTransaction : Transaction { + + /// Initializes a new unsubscribing transaction + /// + /// Transaction whose AsyncEnded event will be monitored to trigger + /// the this transaction unsubscribing from the event. + /// + public UnsubscribingTransaction(Transaction transactionToMonitor) { + this.transactionToMonitor = transactionToMonitor; + this.monitoredTransactionEndedDelegate = new EventHandler( + monitoredTransactionEnded + ); + + this.transactionToMonitor.AsyncEnded += this.monitoredTransactionEndedDelegate; + } + + /// Called when the monitored transaction has ended + /// Monitored transaction that has ended + /// Not used + private void monitoredTransactionEnded(object sender, EventArgs arguments) { + this.transactionToMonitor.AsyncEnded -= this.monitoredTransactionEndedDelegate; + } + + /// Transitions the transaction into the ended state + public void End() { + OnAsyncEnded(); + } + + /// Transaction whose ending in being monitored + private Transaction transactionToMonitor; + /// Delegate to the monitoredTransactionEnded() method + private EventHandler monitoredTransactionEndedDelegate; + + } + + #endregion // class TestWiatable + /// Initialization routine executed before each test is run [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(); + } + } + /// Mocks a subscriber for the events of a transaction /// Transaction to mock an event subscriber for /// The mocked event subscriber diff --git a/Source/Tracking/Transaction.cs b/Source/Tracking/Transaction.cs index 2fa579a..51ea54d 100644 --- a/Source/Tracking/Transaction.cs +++ b/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, - // otherwise, we can be sure that no actual subscribers exist. - if(!ReferenceEquals(this.endedEventSubscribers, null)) { - int eventHandlerIndex = this.endedEventSubscribers.IndexOf(value); + // Only try to remove the event handler if the subscriber list was created, + // otherwise, we can be sure that no actual subscribers exist. + if(!ReferenceEquals(this.endedEventSubscribers, null)) { + 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; } }