From 5c9d5d32d363d073a60fa66663dcf4794985b6a0 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Mon, 15 Oct 2018 15:44:57 -0400 Subject: [PATCH] Null out CancellationTokenSource._timer on Dispose/Cancel (dotnet/coreclr#20410) We already Dispose the Timer in such cases, but we don't null out the field. That's generally fine, unless an errant CancellationToken is held onto somewhere that references the CancellationTokenSource, in which case it in turn may end up keeping the Timer alive and whatever its delegate/state reference, prolonging their GC unnecessarily. Minor, but good house keeping, as CancellationTokens can be used in a manner that makes them longer-lived than expected. Commit migrated from https://github.com/dotnet/coreclr/commit/0af70b614ded7636e6615bf035bbd5149670142b --- .../System/Threading/CancellationTokenSource.cs | 34 +++++++++++++++++----- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs index 291e536..498501d 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs @@ -206,6 +206,11 @@ namespace System.Threading { _state = NotCanceledState; _timer = new Timer(s_timerCallback, this, millisecondsDelay, -1, flowExecutionContext: false); + + // The timer roots this CTS instance while it's scheduled. That is by design, so + // that code like: + // CancellationToken ct = new CancellationTokenSource(timeout).Token; + // will successfully cancel the token after the timeout. } /// Communicates a request for cancellation. @@ -339,25 +344,28 @@ namespace System.Threading // expired and Disposed itself). But this would be considered bad behavior, as // Dispose() is not thread-safe and should not be called concurrently with CancelAfter(). - if (_timer == null) + Timer timer = _timer; + if (timer == null) { // Lazily initialize the timer in a thread-safe fashion. // Initially set to "never go off" because we don't want to take a // chance on a timer "losing" the initialization and then // cancelling the token before it (the timer) can be disposed. - Timer newTimer = new Timer(s_timerCallback, this, -1, -1, flowExecutionContext: false); - if (Interlocked.CompareExchange(ref _timer, newTimer, null) != null) + timer = new Timer(s_timerCallback, this, -1, -1, flowExecutionContext: false); + Timer currentTimer = Interlocked.CompareExchange(ref _timer, timer, null); + if (currentTimer != null) { // We did not initialize the timer. Dispose the new timer. - newTimer.Dispose(); + timer.Dispose(); + timer = currentTimer; } } - // It is possible that m_timer has already been disposed, so we must do + // It is possible that _timer has already been disposed, so we must do // the following in a try/catch block. try { - _timer.Change(millisecondsDelay, -1); + timer.Change(millisecondsDelay, -1); } catch (ObjectDisposedException) { @@ -407,7 +415,12 @@ namespace System.Threading // internal source of cancellation, then Disposes of that linked source, which could // happen at the same time the external entity is requesting cancellation). - _timer?.Dispose(); // Timer.Dispose is thread-safe + Timer timer = _timer; + if (timer != null) + { + _timer = null; + timer.Dispose(); // Timer.Dispose is thread-safe + } _callbackPartitions = null; // free for GC; Cancel correctly handles a null field @@ -552,7 +565,12 @@ namespace System.Threading if (!IsCancellationRequested && Interlocked.CompareExchange(ref _state, NotifyingState, NotCanceledState) == NotCanceledState) { // Dispose of the timer, if any. Dispose may be running concurrently here, but Timer.Dispose is thread-safe. - _timer?.Dispose(); + Timer timer = _timer; + if (timer != null) + { + _timer = null; + timer.Dispose(); + } // 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 -- 2.7.4