From 190e32ee567a2e6908d016a195b294773f3ad993 Mon Sep 17 00:00:00 2001 From: Markus Ewald Date: Thu, 26 Feb 2009 19:47:41 +0000 Subject: [PATCH] Fixed a serious bug in the TransactionGroup which would cause the TransactionGroup to crash when all the transactions it gets passed in the constructor were in the ended state; fixed another serious bug that would occur when transactions reached the ended state during the constructor; wrote unit tests that reproduced those bugs git-svn-id: file:///srv/devel/repo-conversion/nusu@121 d2e56fa2-650e-0410-a79f-9358c0239efd --- .../ObservedWeightedTransaction.Test.cs | 5 +- .../Internal/ObservedWeightedTransaction.cs | 7 +- Source/Tracking/ProgressTracker.Test.cs | 4 +- Source/Tracking/Transaction.Test.cs | 8 +- Source/Tracking/TransactionGroup.Test.cs | 105 ++++++++++++++++++ Source/Tracking/TransactionGroup.cs | 61 +++++++--- 6 files changed, 164 insertions(+), 26 deletions(-) diff --git a/Source/Tracking/Internal/ObservedWeightedTransaction.Test.cs b/Source/Tracking/Internal/ObservedWeightedTransaction.Test.cs index 6005561..1409e35 100644 --- a/Source/Tracking/Internal/ObservedWeightedTransaction.Test.cs +++ b/Source/Tracking/Internal/ObservedWeightedTransaction.Test.cs @@ -98,7 +98,10 @@ namespace Nuclex.Support.Tracking { IObservationSubscriber subscriber = this.mockery.NewMock(); Expect.AtLeast(0).On(subscriber).Method("ProgressUpdated"); - Expect.Once.On(subscriber).Method("Ended"); + // This should no be called because otherwise, the 'Ended' event would be raised + // to the transaction group before all transactions have been added into + // the internal list, leading to an early ending or even multiple endings. + Expect.Never.On(subscriber).Method("Ended"); using( ObservedWeightedTransaction test = diff --git a/Source/Tracking/Internal/ObservedWeightedTransaction.cs b/Source/Tracking/Internal/ObservedWeightedTransaction.cs index ad6e8ee..8e3d585 100644 --- a/Source/Tracking/Internal/ObservedWeightedTransaction.cs +++ b/Source/Tracking/Internal/ObservedWeightedTransaction.cs @@ -58,7 +58,12 @@ namespace Nuclex.Support.Tracking { // prevent object coupling where none is neccessary and to save some processing time. this.progress = 1.0f; progressUpdateCallback(); - endedCallback(); + + // Do not call the ended callback here. This constructor is called when the + // TransactionGroup constructs its list of transactions. If this is called and + // the first transaction to be added to the group happens to be in the ended + // state, the transactionGroup will immediately think it has ended! + //!DONT!endedCallback(); return; diff --git a/Source/Tracking/ProgressTracker.Test.cs b/Source/Tracking/ProgressTracker.Test.cs index 79ec785..ef76a35 100644 --- a/Source/Tracking/ProgressTracker.Test.cs +++ b/Source/Tracking/ProgressTracker.Test.cs @@ -18,12 +18,12 @@ License along with this library */ #endregion +#if UNITTEST + using System; using System.Collections.Generic; using System.IO; -#if UNITTEST - using NUnit.Framework; using NMock2; diff --git a/Source/Tracking/Transaction.Test.cs b/Source/Tracking/Transaction.Test.cs index fd6d15e..46ce19a 100644 --- a/Source/Tracking/Transaction.Test.cs +++ b/Source/Tracking/Transaction.Test.cs @@ -18,13 +18,13 @@ License along with this library */ #endregion +#if UNITTEST + using System; using System.Collections.Generic; using System.IO; using System.Threading; -#if UNITTEST - using NUnit.Framework; using NMock2; @@ -92,7 +92,7 @@ namespace Nuclex.Support.Tracking { WithAnyArguments(); test.End(); - + this.mockery.VerifyAllExpectationsHaveBeenMet(); } @@ -140,7 +140,7 @@ namespace Nuclex.Support.Tracking { // Wait 0 milliseconds for the transaction to end. Of course, this will not happen, // so a timeout occurs and false is returned Assert.IsFalse(test.Wait(0)); - + test.End(); // Wait another 0 milliseconds for the transaction to end. Now it has already ended diff --git a/Source/Tracking/TransactionGroup.Test.cs b/Source/Tracking/TransactionGroup.Test.cs index 615cff4..a1b778d 100644 --- a/Source/Tracking/TransactionGroup.Test.cs +++ b/Source/Tracking/TransactionGroup.Test.cs @@ -21,6 +21,7 @@ License along with this library using System; using System.Collections.Generic; using System.IO; +using System.Threading; #if UNITTEST @@ -139,6 +140,53 @@ namespace Nuclex.Support.Tracking { #endregion // class TestTransaction + #region class ChainEndingTransaction + + /// + /// Transaction that ends another transaction when its Ended property is called + /// + private class ChainEndingTransaction : Transaction { + + /// Initializes a new chain ending transaction + public ChainEndingTransaction() { + this.chainedTransaction = new TestTransaction(); + } + + /// Transitions the transaction into the ended state + public void End() { + OnAsyncEnded(); + } + + /// + /// Transaction that will end when this transaction's ended property is accessed + /// + public TestTransaction ChainedTransaction { + get { return this.chainedTransaction; } + } + + /// Whether the transaction has ended already + public override bool Ended { + get { + if(Interlocked.Exchange(ref this.endedCalled, 1) == 0) { + this.chainedTransaction.End(); + } + + return base.Ended; + } + } + + /// + /// Transaction that will end when this transaction's ended property is accessed + /// + private TestTransaction chainedTransaction; + + /// Whether we already ended the chained transaction and ourselves + private int endedCalled; + + } + + #endregion // class ChainEndingTransaction + /// Initialization routine executed before each test is run [SetUp] public void Setup() { @@ -239,6 +287,63 @@ namespace Nuclex.Support.Tracking { } } + /// + /// Verifies that the transaction group immediately enters the ended state when + /// the contained transactions have already ended before the constructor + /// + /// + /// This was a bug at one time and should prevent a regression + /// + [Test] + public void TestAlreadyEndedTransactions() { + using( + TransactionGroup testTransactionGroup = + new TransactionGroup( + new Transaction[] { Transaction.EndedDummy, Transaction.EndedDummy } + ) + ) { + Assert.IsTrue(testTransactionGroup.Wait(1000)); + } + } + + /// + /// Verifies that the transaction group doesn't think it's already ended when + /// the first transaction being added is in the ended state + /// + /// + /// This was a bug at one time and should prevent a regression + /// + [Test] + public void TestAlreadyEndedTransactionAsFirstTransaction() { + using( + TransactionGroup testTransactionGroup = + new TransactionGroup( + new Transaction[] { Transaction.EndedDummy, new TestTransaction() } + ) + ) { + Assert.IsFalse(testTransactionGroup.Ended); + } + } + + /// + /// Verifies that a transaction ending while the constructor is running doesn't + /// wreak havoc on the transaction group + /// + [Test] + public void TestTransactionEndingDuringConstructor() { + ChainEndingTransaction chainTransaction = new ChainEndingTransaction(); + using( + TransactionGroup testTransactionGroup = + new TransactionGroup( + new Transaction[] { chainTransaction.ChainedTransaction, chainTransaction } + ) + ) { + Assert.IsFalse(testTransactionGroup.Ended); + chainTransaction.End(); + Assert.IsTrue(testTransactionGroup.Ended); + } + } + /// 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/TransactionGroup.cs b/Source/Tracking/TransactionGroup.cs index 5770da4..d67bd4b 100644 --- a/Source/Tracking/TransactionGroup.cs +++ b/Source/Tracking/TransactionGroup.cs @@ -21,6 +21,7 @@ License along with this library using System; using System.Collections.Generic; using System.Collections.ObjectModel; +using System.Threading; using Nuclex.Support.Collections; @@ -35,17 +36,18 @@ namespace Nuclex.Support.Tracking { public event EventHandler AsyncProgressChanged; /// Initializes a new transaction group - /// Transactions to track with this group + /// Transactions to track with this group /// /// Uses a default weighting factor of 1.0 for all transactions. /// - public TransactionGroup(IEnumerable childs) - : this() { + public TransactionGroup(IEnumerable children) { + List> childrenList = + new List>(); // Construct a WeightedTransaction with the default weight for each // transaction and wrap it in an ObservedTransaction - foreach(TransactionType transaction in childs) { - this.children.Add( + foreach(TransactionType transaction in children) { + childrenList.Add( new ObservedWeightedTransaction( new WeightedTransaction(transaction), new ObservedWeightedTransaction.ReportDelegate( @@ -60,20 +62,26 @@ namespace Nuclex.Support.Tracking { // Since all transactions have a weight of 1.0, the total weight is // equal to the number of transactions in our list - this.totalWeight = (float)this.children.Count; + this.totalWeight = (float)childrenList.Count; + // Thread.MemoryBarrier(); // not needed because children is volatile + this.children = childrenList; + // Any asyncEnded events being receiving from the transactions until now + // would have been ignored, so we need to check again here + asyncChildEnded(); } /// Initializes a new transaction group - /// Transactions to track with this group + /// Transactions to track with this group public TransactionGroup( - IEnumerable> childs - ) - : this() { + IEnumerable> children + ) { + List> childrenList = + new List>(); // Construct an ObservedTransaction around each of the WeightedTransactions - foreach(WeightedTransaction transaction in childs) { - this.children.Add( + foreach(WeightedTransaction transaction in children) { + childrenList.Add( new ObservedWeightedTransaction( transaction, new ObservedWeightedTransaction.ReportDelegate( @@ -89,11 +97,11 @@ namespace Nuclex.Support.Tracking { this.totalWeight += transaction.Weight; } - } + this.children = childrenList; - /// Performs common initialization for the public constructors - private TransactionGroup() { - this.children = new List>(); + // Any asyncEnded events being receiving from the transactions until now + // would have been ignored, so we need to check again here + asyncChildEnded(); } /// Immediately releases all resources owned by the object @@ -162,6 +170,10 @@ namespace Nuclex.Support.Tracking { /// Called when the progress of one of the observed transactions changes /// private void asyncProgressUpdated() { + if(this.children == null) { + return; + } + float totalProgress = 0.0f; // Calculate the sum of the progress reported by our child transactions, @@ -184,6 +196,15 @@ namespace Nuclex.Support.Tracking { /// private void asyncChildEnded() { + // If a transaction reports its end durign the constructor, it will end up here + // where the collection has not been assigned yet, allowing us to skip the + // check until all transactions are there (otherwise, we might invoke + // OnAsyncended() early, because all transactions in the list seem to have ended + // despite the fact that the constructor hasn't finished adding transactions yet) + if(this.children == null) { + return; + } + // If there's still at least one transaction going, don't report that // the transaction group has finished yet. for(int index = 0; index < this.children.Count; ++index) @@ -191,12 +212,14 @@ namespace Nuclex.Support.Tracking { return; // All child transactions have ended, so the set has now ended as well - OnAsyncEnded(); + if(Interlocked.Exchange(ref this.endedCalled, 1) == 0) { + OnAsyncEnded(); + } } /// Transactions being managed in the set - private List> children; + private volatile List> children; /// /// Wrapper collection for exposing the child transactions under the /// WeightedTransaction interface @@ -204,6 +227,8 @@ namespace Nuclex.Support.Tracking { private volatile WeightedTransactionWrapperCollection wrapper; /// Summed weight of all transactions in the set private float totalWeight; + /// Whether we already called OnAsyncEnded + private int endedCalled; }