Fix CancellationTokenRegistration.Dispose racing with cancellation (dotnet/coreclr...
authorStephen Toub <stoub@microsoft.com>
Thu, 27 Sep 2018 18:58:18 +0000 (14:58 -0400)
committerGitHub <noreply@github.com>
Thu, 27 Sep 2018 18:58:18 +0000 (14:58 -0400)
commit9f97fa53f639b5b8dff8764448618503882d1825
treef04d95827823bb3490a4a7376fff19e0b375a06b
parent3d019744ef9391b8f17cdef46c7c0a0e4749b4c2
Fix CancellationTokenRegistration.Dispose racing with cancellation (dotnet/coreclr#20145)

CancellationTokenRegistration.Dispose is guaranteed to only return when the associated callback has already finished running or has been successfully removed such that it'll never run.  This is to ensure that code following Dispose can be guaranteed that the callback isn't currently running and won't after that point, as if it did, it could potentially depend on mutable shared state or itself mutate shared state in a non-thread-safe manner.

However, significant optimizations introduced in .NET Core 2.1 impacted a specific case of this guarantee.  It still behaves correctly if cancellation hasn't already been requested, if cancellation has already processed the associated callback, and even if cancellation is currently executing the associated callback.  However, if cancellation is currently processing other callbacks and hasn't yet gotten around to processing the associated one, Dispose() may incorrectly return immediately, such that the callback may still end up getting invoked after Dispose() returns, which can violate assumptions of consuming code in very impactful ways.

This commit modifies how the callbacks are removed from the registration list in order to fix the issue.  Previously, all of the callbacks were swapped out at once, which then left the list empty, which is what caused subsequent disposals to think the callback had already been processed or unregistered.  With this change, we instead just remove each registration as it's being processed, such that a concurrent disposal can still successfully find the registration in the callback list if it's not yet been processed.

This change does have a small perf impact, but only on the case where cancellation is actually invoked, which is the less common case; most usage of CTS doesn't actually result in cancellation, and optimization is focused on registration and unregistration perf.  Rather than taking and releasing the lock once when cancellation is requested, we now take and release the lock per callback when cancellation is requested.

Commit migrated from https://github.com/dotnet/coreclr/commit/552f4bf7addc944d85f4604fd2fe228ac984412d
src/coreclr/src/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs