Fix race condition between CTS.Cancel and Dispose
authorstephentoub <stoub@microsoft.com>
Fri, 8 Apr 2016 17:01:19 +0000 (13:01 -0400)
committerstephentoub <stoub@microsoft.com>
Sun, 10 Apr 2016 11:39:31 +0000 (07:39 -0400)
commitfa544fdcaed4ae10ecab53153b2a8b5949fb8585
tree20b08f5f934c47be1739f23ae44d596341434ea9
parentf268b3e4902cc21013204581d52f6ebbdd15a008
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.

Commit migrated from https://github.com/dotnet/coreclr/commit/3a6e9459c0b5dc69f898272b290ce0333cebd55b
src/coreclr/src/mscorlib/src/System/Threading/CancellationTokenSource.cs