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

Source/Tracking/Internal/ObservedWeightedTransaction.Test.cs

@@ -98,7 +98,10 @@ namespace Nuclex.Support.Tracking {
       IObservationSubscriber subscriber = this.mockery.NewMock<IObservationSubscriber>();
 
       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<Transaction> test =

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;
 

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;
 

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

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
+
+    /// <summary>
+    ///   Transaction that ends another transaction when its Ended property is called
+    /// </summary>
+    private class ChainEndingTransaction : Transaction {
+
+      /// <summary>Initializes a new chain ending transaction</summary>
+      public ChainEndingTransaction() {
+        this.chainedTransaction = new TestTransaction();
+      }
+
+      /// <summary>Transitions the transaction into the ended state</summary>
+      public void End() {
+        OnAsyncEnded();
+      }
+
+      /// <summary>
+      ///   Transaction that will end when this transaction's ended property is accessed
+      /// </summary>
+      public TestTransaction ChainedTransaction {
+        get { return this.chainedTransaction; }
+      }
+
+      /// <summary>Whether the transaction has ended already</summary>
+      public override bool Ended {
+        get {
+          if(Interlocked.Exchange(ref this.endedCalled, 1) == 0) {
+            this.chainedTransaction.End();
+          }
+
+          return base.Ended;
+        }
+      }
+
+      /// <summary>
+      ///   Transaction that will end when this transaction's ended property is accessed
+      /// </summary>
+      private TestTransaction chainedTransaction;
+
+      /// <summary>Whether we already ended the chained transaction and ourselves</summary>
+      private int endedCalled;
+
+    }
+
+    #endregion // class ChainEndingTransaction
+
     /// <summary>Initialization routine executed before each test is run</summary>
     [SetUp]
     public void Setup() {
@@ -239,6 +287,63 @@ namespace Nuclex.Support.Tracking {
       }
     }
 
+    /// <summary>
+    ///   Verifies that the transaction group immediately enters the ended state when
+    ///   the contained transactions have already ended before the constructor
+    /// </summary>
+    /// <remarks>
+    ///   This was a bug at one time and should prevent a regression
+    /// </remarks>
+    [Test]
+    public void TestAlreadyEndedTransactions() {
+      using(
+        TransactionGroup<Transaction> testTransactionGroup =
+          new TransactionGroup<Transaction>(
+            new Transaction[] { Transaction.EndedDummy, Transaction.EndedDummy }
+          )
+      ) {
+        Assert.IsTrue(testTransactionGroup.Wait(1000));
+      }
+    }
+
+    /// <summary>
+    ///   Verifies that the transaction group doesn't think it's already ended when
+    ///   the first transaction being added is in the ended state
+    /// </summary>
+    /// <remarks>
+    ///   This was a bug at one time and should prevent a regression
+    /// </remarks>
+    [Test]
+    public void TestAlreadyEndedTransactionAsFirstTransaction() {
+      using(
+        TransactionGroup<Transaction> testTransactionGroup =
+          new TransactionGroup<Transaction>(
+            new Transaction[] { Transaction.EndedDummy, new TestTransaction() }
+          )
+      ) {
+        Assert.IsFalse(testTransactionGroup.Ended);
+      }
+    }
+
+    /// <summary>
+    ///   Verifies that a transaction ending while the constructor is running doesn't
+    ///   wreak havoc on the transaction group
+    /// </summary>
+    [Test]
+    public void TestTransactionEndingDuringConstructor() {
+      ChainEndingTransaction chainTransaction = new ChainEndingTransaction();
+      using(
+        TransactionGroup<Transaction> testTransactionGroup =
+          new TransactionGroup<Transaction>(
+            new Transaction[] { chainTransaction.ChainedTransaction, chainTransaction }
+          )
+      ) {
+        Assert.IsFalse(testTransactionGroup.Ended);
+        chainTransaction.End();
+        Assert.IsTrue(testTransactionGroup.Ended);
+      }
+    }
+
     /// <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/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<ProgressReportEventArgs> AsyncProgressChanged;
 
     /// <summary>Initializes a new transaction group</summary>
-    /// <param name="childs">Transactions to track with this group</param>
+    /// <param name="children">Transactions to track with this group</param>
     /// <remarks>
     ///   Uses a default weighting factor of 1.0 for all transactions.
     /// </remarks>
-    public TransactionGroup(IEnumerable<TransactionType> childs)
-      : this() {
+    public TransactionGroup(IEnumerable<TransactionType> children) {
+      List<ObservedWeightedTransaction<TransactionType>> childrenList =
+        new List<ObservedWeightedTransaction<TransactionType>>();
 
       // 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<TransactionType>(
             new WeightedTransaction<TransactionType>(transaction),
             new ObservedWeightedTransaction<TransactionType>.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();
     }
 
     /// <summary>Initializes a new transaction group</summary>
-    /// <param name="childs">Transactions to track with this group</param>
+    /// <param name="children">Transactions to track with this group</param>
     public TransactionGroup(
-      IEnumerable<WeightedTransaction<TransactionType>> childs
-    )
-      : this() {
+      IEnumerable<WeightedTransaction<TransactionType>> children
+    ) {
+      List<ObservedWeightedTransaction<TransactionType>> childrenList =
+        new List<ObservedWeightedTransaction<TransactionType>>();
 
       // Construct an ObservedTransaction around each of the WeightedTransactions
-      foreach(WeightedTransaction<TransactionType> transaction in childs) {
-        this.children.Add(
+      foreach(WeightedTransaction<TransactionType> transaction in children) {
+        childrenList.Add(
           new ObservedWeightedTransaction<TransactionType>(
             transaction,
             new ObservedWeightedTransaction<TransactionType>.ReportDelegate(
@@ -89,11 +97,11 @@ namespace Nuclex.Support.Tracking {
         this.totalWeight += transaction.Weight;
       }
 
-    }
+      this.children = childrenList;
 
-    /// <summary>Performs common initialization for the public constructors</summary>
-    private TransactionGroup() {
-      this.children = new List<ObservedWeightedTransaction<TransactionType>>();
+      // Any asyncEnded events being receiving from the transactions until now
+      // would have been ignored, so we need to check again here
+      asyncChildEnded();
     }
 
     /// <summary>Immediately releases all resources owned by the object</summary>
@@ -162,6 +170,10 @@ namespace Nuclex.Support.Tracking {
     ///   Called when the progress of one of the observed transactions changes
     /// </summary>
     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 {
     /// </summary>
     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();
+      }
 
     }
 
     /// <summary>Transactions being managed in the set</summary>
-    private List<ObservedWeightedTransaction<TransactionType>> children;
+    private volatile List<ObservedWeightedTransaction<TransactionType>> children;
     /// <summary>
     ///   Wrapper collection for exposing the child transactions under the
     ///   WeightedTransaction interface
@@ -204,6 +227,8 @@ namespace Nuclex.Support.Tracking {
     private volatile WeightedTransactionWrapperCollection<TransactionType> wrapper;
     /// <summary>Summed weight of all transactions in the set</summary>
     private float totalWeight;
+    /// <summary>Whether we already called OnAsyncEnded</summary>
+    private int endedCalled;
 
   }