Fix race condition between CTS.Cancel and Dispose

In general, CancellationTokenSource.Dispose is not meant to be used concurrently with other operations on the instance: Dispose should only be used when the creator of the CTS is done with it.  However, there's one common situation where recommended usage runs afoul of this.  A typical usage of a LinkedCancellationTokenSource (as created by CancellationTokenSource.CreateLinkedTokenSource) is as follows:
```C#
private CancellationToken _internalCancellation;

public void SomeMethod(CancellationToken externalCancellation)
{
    using (var cts = CancellationTokenSource.CreateLinkedTokenSource(externalCancellation, _internalCancellation))
    {
        DoWork(cts.Token);
    }
}
```
where an LCTS is used to combine some external source of cancellation with some internal one.  It's important that the LCTS then be disposed, as otherwise it'll end up leaking state into constituent CTs, and in particular in the case of the externally provided one, that could be a very long lived token.  However, it's perfectly fine for the external token to have cancellation requested at any time during this method, which means that even though Dispose and Cancel weren't designed to be safe to execute concurrently, and even though we recommend against it, it's actually possible with recommended usage.

It's a very dificult race to reproduce, but when it does, the typical visible result is an ObjectDisposedException.  State corruption could also result.

This commit addresses that in two ways:
1. Rather than having the LCTS register a delegate with each CT that calls Cancel, it instead has its delegate call NotifyCancellation.  This is the same method Cancel calls, but without the upfront disposal check.  That eliminates the easily visible result of a race that would cause an ObjectDisposedException.
2. With all of the tweaking that's been done to Dispose since it was originally written, it's actually very close to being safe to use concurrently with Cancel; this takes it the rest of the way (though we still don't want to document it as such, it's really just to make LCTS work correctly).  The only state Dispose mutates that could be problematic is a Timer and a ManualResetEvent.  Timer's Dispose is already thread-safe.  MRE's isn't, so if it's been allocated (which is relatively rare), I add a single Interlocked.Exchange to null it out, and then do a volatile read on a state field to see whether cancellation is currently in progress, only Dispose'ing of the MRE if it is, and leaving it for finalization otherwise.  On the Cancel side, we just add some null checks to ensure we're not attempting to Dispose of null'd out fields, again with volatile reads where necessary.

diff --git a/src/mscorlib/src/System/Threading/CancellationTokenSource.cs b/src/mscorlib/src/System/Threading/CancellationTokenSource.cs
index 4874804..954cd38 100644 (file)
--- a/src/mscorlib/src/System/Threading/CancellationTokenSource.cs
+++ b/src/mscorlib/src/System/Threading/CancellationTokenSource.cs
@@ -556,18 +556,33 @@ namespace System.Threading
                 //      instance may unnecessarily hold onto a registered callback.  But that's no worse
                 //      than if Dispose wasn't safe to use concurrently, as Dispose would never be called,
                 //      and thus no handlers would be dropped.
+                //
+                //      And, we tolerate Dispose being used concurrently with Cancel.  This is necessary
+                //      to properly support LinkedCancellationTokenSource, where, due to common usage patterns,
+                //      it's possible for this pairing to occur with valid usage (e.g. a component accepts
+                //      an external CancellationToken and uses CreateLinkedTokenSource to combine it with an
+                //      internal source of cancellation, then Disposes of that linked source, which could
+                //      happen at the same time the external entity is requesting cancellation).
 
-                if (m_timer != null) m_timer.Dispose();
+                m_timer?.Dispose(); // Timer.Dispose is thread-safe
 
                 // registered callbacks are now either complete or will never run, due to guarantees made by ctr.Dispose()
                 // so we can now perform main disposal work without risk of linking callbacks trying to use this CTS.
 
-                m_registeredCallbacksLists = null; // free for GC.
+                m_registeredCallbacksLists = null; // free for GC; Cancel correctly handles a null field
 
+                // If a kernel event was created via WaitHandle, we'd like to Dispose of it.  However,
+                // we only want to do so if it's not being used by Cancel concurrently.  First, we
+                // interlocked exchange it to be null, and then we check whether cancellation is currently
+                // in progress.  NotifyCancellation will only try to set the event if it exists after it's
+                // transitioned to and while it's in the NOTIFYING state.
                 if (m_kernelEvent != null)
                 {
-                    m_kernelEvent.Close(); // the critical cleanup to release an OS handle
-                    m_kernelEvent = null; // free for GC.
+                    ManualResetEvent mre = Interlocked.Exchange(ref m_kernelEvent, null);
+                    if (mre != null && m_state != NOTIFYING)
+                    {
+                        mre.Dispose();
+                    }
                 }
 
                 m_disposed = true;
@@ -698,16 +713,17 @@ namespace System.Threading
             // If we're the first to signal cancellation, do the main extra work.
             if (Interlocked.CompareExchange(ref m_state, NOTIFYING, NOT_CANCELED) == NOT_CANCELED)
             {
-                // Dispose of the timer, if any
-                Timer timer = m_timer;
-                if(timer != null) timer.Dispose();
+                // Dispose of the timer, if any.  Dispose may be running concurrently here, but Timer.Dispose is thread-safe.
+                m_timer?.Dispose();
 
-                //record the threadID being used for running the callbacks.
+                // Record the threadID being used for running the callbacks.
                 ThreadIDExecutingCallbacks = Thread.CurrentThread.ManagedThreadId;
                 
-                //If the kernel event is null at this point, it will be set during lazy construction.
-                if (m_kernelEvent != null)
-                    m_kernelEvent.Set(); // update the MRE value.
+                // Set the event if it's been lazily initialized and hasn't yet been disposed of.  Dispose may
+                // be running concurrently, in which case either it'll have set m_kernelEvent back to null and
+                // we won't see it here, or it'll see that we've transitioned to NOTIFYING and will skip disposing it,
+                // leaving cleanup to finalization.
+                m_kernelEvent?.Set(); // update the MRE value.
 
                 // - late enlisters to the Canceled event will have their callbacks called immediately in the Register() methods.
                 // - Callbacks are not called inside a lock.
@@ -893,7 +909,8 @@ namespace System.Threading
 
         private sealed class LinkedCancellationTokenSource : CancellationTokenSource
         {
-            private static readonly Action<object> s_linkedTokenCancelDelegate = s => ((CancellationTokenSource)s).Cancel();
+            private static readonly Action<object> s_linkedTokenCancelDelegate = 
+                s => ((CancellationTokenSource)s).NotifyCancellation(throwOnFirstException: false); // skip ThrowIfDisposed() check in Cancel()
             private CancellationTokenRegistration[] m_linkingRegistrations;
 
             internal LinkedCancellationTokenSource(CancellationToken token1, CancellationToken token2)