Fix race condition in AsyncOperation

The AsyncOperation type in System.ComponentModel.EventBasedAsync tracks whether it's already been completed, and any subsequent attempts to Post operations through a completed instance are supposed to result in exceptions.  However, the implementation currently doesn't set its completion flag until after it's invoked the completion callback.  This means that if the completion callback then itself attempts to reuse the instance or if it initiates another async operation that does, there's a race condition between the subsequent access to the instance and the setting of the flag. This change fixes up the ordering so that the flag is set prior to invoking the completion callback rather than after.

Author: stephentoub

src/System.ComponentModel.EventBasedAsync/src/System/ComponentModel/AsyncOperation.cs

@@ -53,23 +53,37 @@ public SynchronizationContext SynchronizationContext
 
         public void Post(SendOrPostCallback d, object arg)
         {
-            VerifyNotCompleted();
-            VerifyDelegateNotNull(d);
-            _syncContext.Post(d, arg);
+            PostCore(d, arg, markCompleted: false);
         }
 
         public void PostOperationCompleted(SendOrPostCallback d, object arg)
         {
-            Post(d, arg);
+            PostCore(d, arg, markCompleted: true);
             OperationCompletedCore();
         }
 
         public void OperationCompleted()
         {
             VerifyNotCompleted();
+            _alreadyCompleted = true;
             OperationCompletedCore();
         }
 
+        private void PostCore(SendOrPostCallback d, object arg, bool markCompleted)
+        {
+            VerifyNotCompleted();
+            VerifyDelegateNotNull(d);
+            if (markCompleted)
+            {
+                // This call is in response to a PostOperationCompleted.  As such, we need to mark
+                // _alreadyCompleted as true so that subsequent attempts to use this instance will
+                // fail appropriately.  And we need to do that before we queue the callback, as
+                // that callback could itself trigger additional attempts to use this instance.
+                _alreadyCompleted = true;
+            }
+            _syncContext.Post(d, arg);
+        }
+
         private void OperationCompletedCore()
         {
             try
@@ -78,7 +92,6 @@ private void OperationCompletedCore()
             }
             finally
             {
-                _alreadyCompleted = true;
                 GC.SuppressFinalize(this);
             }
         }