Fixed a critical threading bug in the weighted transaction observation wrapper; ProgressTracker not has 100% test coverage; wrote even more unit tests for the ProgressTracker

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

Source/Tracking/Internal/ObservedWeightedTransaction.cs

@@ -126,10 +126,14 @@ namespace Nuclex.Support.Tracking {
 
     /// <summary>Called when the progress of the observed transaction changes</summary>
     /// <param name="sender">Transaction whose progress has changed</param>
-    /// <param name="e">Contains the updated progress</param>
-    private void asyncProgressChanged(object sender, ProgressReportEventArgs e) {
-      this.progress = e.Progress;
-      this.progressUpdateCallback();
+    /// <param name="arguments">Contains the updated progress</param>
+    private void asyncProgressChanged(object sender, ProgressReportEventArgs arguments) {
+      this.progress = arguments.Progress;
+      
+      ReportDelegate savedProgressUpdateCallback = this.progressUpdateCallback;
+      if(savedProgressUpdateCallback != null) {
+        savedProgressUpdateCallback();
+      }
     }
 
     /// <summary>Unsubscribes from all events of the observed transaction</summary>

Source/Tracking/ProgressTracker.Test.cs

@@ -135,7 +135,28 @@ namespace Nuclex.Support.Tracking {
 
     }
 
-    #endregion // class TestWiatable
+    #endregion // class TestTransaction
+
+    #region class EvilTransaction
+
+    /// <summary>
+    ///   Transaction that tries to emulate a thread given a progress report at
+    ///   a very inconvenient time ;)
+    /// </summary>
+    private class EvilTransaction : Transaction, IProgressReporter {
+
+      /// <summary>will be triggered to report when progress has been achieved</summary>
+      public event EventHandler<ProgressReportEventArgs> AsyncProgressChanged {
+        add {}
+        remove {
+          // Send a progress update right when the subscriber is trying to unsubscribe
+          value(this, new ProgressReportEventArgs(0.5f));
+        }
+      }
+
+    }
+
+    #endregion // class EvilTransaction
 
     /// <summary>Initialization routine executed before each test is run</summary>
     [SetUp]
@@ -380,6 +401,115 @@ namespace Nuclex.Support.Tracking {
       }
     }
 
+    /// <summary>
+    ///   Tests whether the progress tracker enters and leaves the idle state correctly
+    ///   when a transaction is removed via Untrack()
+    /// </summary>
+    [Test]
+    public void TestIdleWithUntrack() {
+      using(ProgressTracker tracker = new ProgressTracker()) {
+        TestTransaction test1 = new TestTransaction();
+
+        Assert.IsTrue(tracker.Idle);
+
+        tracker.Track(test1);
+
+        Assert.IsFalse(tracker.Idle);
+
+        tracker.Untrack(test1);
+
+        Assert.IsTrue(tracker.Idle);
+      }
+    }
+
+    /// <summary>
+    ///   Tests whether the progress tracker enters and leaves the idle state correctly
+    ///   when a transaction is removed the transaction finishing
+    /// </summary>
+    [Test]
+    public void TestIdleWithAutoRemoval() {
+      using(ProgressTracker tracker = new ProgressTracker()) {
+        TestTransaction test1 = new TestTransaction();
+
+        Assert.IsTrue(tracker.Idle);
+
+        tracker.Track(test1);
+
+        Assert.IsFalse(tracker.Idle);
+
+        test1.End();
+
+        Assert.IsTrue(tracker.Idle);
+      }
+    }
+
+    /// <summary>
+    ///   Tests whether the progress tracker enters and leaves the idle state correctly
+    ///   when a transaction is removed via Untrack()
+    /// </summary>
+    [Test]
+    public void TestProgressWithUntrack() {
+      using(ProgressTracker tracker = new ProgressTracker()) {
+        TestTransaction test1 = new TestTransaction();
+        TestTransaction test2 = new TestTransaction();
+        tracker.Track(test1);
+        tracker.Track(test2);
+
+        Assert.AreEqual(0.0f, tracker.Progress);
+
+        test1.ChangeProgress(0.5f);
+
+        Assert.AreEqual(0.25f, tracker.Progress);
+
+        tracker.Untrack(test2);
+
+        Assert.AreEqual(0.5f, tracker.Progress);
+      }
+    }
+
+    /// <summary>
+    ///   Verifies that the progress tracker throws an exception if it is instructed
+    ///   to untrack a transaction it doesn't know about
+    /// </summary>
+    [Test, ExpectedException(typeof(ArgumentException))]
+    public void TestThrowOnUntrackNonTrackedTransaction() {
+      using(ProgressTracker tracker = new ProgressTracker()) {
+        TestTransaction test1 = new TestTransaction();
+
+        tracker.Untrack(test1);
+      }
+    }
+
+    /// <summary>
+    ///   Verifies that the progress tracker throws an exception if it is instructed
+    ///   to untrack a transaction it doesn't know about
+    /// </summary>
+    [Test]
+    public void TestProgressReportDuringUnsubscribe() {
+      using(ProgressTracker tracker = new ProgressTracker()) {
+        EvilTransaction evil = new EvilTransaction();
+        tracker.Track(evil);
+        tracker.Untrack(evil);
+      }
+    }
+
+    /// <summary>
+    ///   Verifies that the progress tracker doesn't choke on a transaction being tracked
+    ///   multiple times.
+    /// </summary>
+    [Test]
+    public void TestMultiTrackedTransaction() {
+      using(ProgressTracker tracker = new ProgressTracker()) {
+        TestTransaction test = new TestTransaction();
+        tracker.Track(test);
+        tracker.Track(test);
+        tracker.Track(test);
+        tracker.Untrack(test);
+        tracker.Untrack(test);
+        tracker.Untrack(test);
+      }
+    }
+
     /// <summary>Mocks a subscriber for the events of a tracker</summary>
     /// <param name="tracker">Tracker to mock an event subscriber for</param>
     /// <returns>The mocked event subscriber</returns>

Source/Tracking/ProgressTracker.cs

@@ -156,7 +156,7 @@ namespace Nuclex.Support.Tracking {
 
           }
 
-        } else { // Not ended -- Transation is still running
+        } else { // Not ended -- Transaction is still running
 
           // Construct a new transation observer and add the transaction to our
           // list of tracked transactions.
@@ -171,8 +171,9 @@ namespace Nuclex.Support.Tracking {
 
           // If this is the first transaction to be added to the list, tell our
           // owner that we're idle no longer!
-          if(wasEmpty)
+          if(wasEmpty) {
             setIdle(false);
+          }
 
         } // if transaction ended
 
@@ -200,8 +201,9 @@ namespace Nuclex.Support.Tracking {
             new TransactionMatcher(transaction).Matches
           )
         );
-        if(removeIndex == -1)
-          throw new InvalidOperationException("Item is not being tracked");
+        if(removeIndex == -1) {
+          throw new ArgumentException("Specified transaction is not being tracked");
+        }
 
         // Remove and dispose the transaction the user wants to untrack
         {
@@ -225,9 +227,13 @@ namespace Nuclex.Support.Tracking {
           // Rebuild the total weight from scratch. Subtracting the removed transaction's
           // weight would work, too, but we might accumulate rounding errors making the sum
           // drift slowly away from the actual value.
-          this.totalWeight = 0.0f;
+          float newTotalWeight = 0.0f;
           for(int index = 0; index < this.trackedTransactions.Count; ++index)
-            this.totalWeight += this.trackedTransactions[index].WeightedTransaction.Weight;
+            newTotalWeight += this.trackedTransactions[index].WeightedTransaction.Weight;
+
+          this.totalWeight = newTotalWeight;
+
+          recalculateProgress();
 
         }
 
@@ -262,7 +268,7 @@ namespace Nuclex.Support.Tracking {
 
     /// <summary>Recalculates the total progress of the tracker</summary>
     private void recalculateProgress() {
-      float totalProgress = 0.0f;
+      bool progressChanged = false;
 
       // Lock the collection to avoid trouble when someone tries to remove one
       // of our tracked transactions while we're just doing a progress update
@@ -272,8 +278,8 @@ namespace Nuclex.Support.Tracking {
         // ended and the collection of tracked transactions is cleared, a waiting
         // thread might deliver another progress update causing this method to
         // be entered. In this case, the right thing is to do nothing at all.
-        if(this.totalWeight == 0.0f)
-          return;
+        if(this.totalWeight != 0.0f) {
+          float totalProgress = 0.0f;
 
           // Sum up the total progress
           for(int index = 0; index < this.trackedTransactions.Count; ++index) {
@@ -281,18 +287,23 @@ namespace Nuclex.Support.Tracking {
             totalProgress += this.trackedTransactions[index].Progress * weight;
           }
 
-        // This also needs to be in the lock to guarantee that the totalWeight is
-        // the one for the number of transactions we just summed -- by design,
+          // This also needs to be in the lock to guarantee that the total weight
+          // corresponds to the number of transactions we just summed -- by design,
           // the total weight always has to be updated at the same time as the collection.
           totalProgress /= this.totalWeight;
 
-        // Finally, trigger the event if the progress has changed
           if(totalProgress != this.progress) {
             this.progress = totalProgress;
-          OnAsyncProgressUpdated(totalProgress);
+            progressChanged = true;
+          }
         }
 
       } // lock
+
+      // Finally, trigger the event if the progress has changed
+      if(progressChanged) {
+        OnAsyncProgressUpdated(this.progress);
+      }
     }
 
     /// <summary>Called when one of the tracked transactions has ended</summary>