From b96b56efe513fcbbe497339548da9d96f897d598 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Fri, 17 Apr 2020 01:14:53 -0700 Subject: [PATCH 1/2] Fix TimerAwaitable rooting - This fixes an issue where components that use timer awaitable currently need to be explicitly stopped and disposed for it to be unrooted. This makes it possible to write a finalizer. --- .../test/UnitTests/TimerAwaitableTests.cs | 66 +++++++++++++++++++ src/SignalR/common/Shared/TimerAwaitable.cs | 17 ++++- 2 files changed, 81 insertions(+), 2 deletions(-) create mode 100644 src/SignalR/clients/csharp/Client/test/UnitTests/TimerAwaitableTests.cs diff --git a/src/SignalR/clients/csharp/Client/test/UnitTests/TimerAwaitableTests.cs b/src/SignalR/clients/csharp/Client/test/UnitTests/TimerAwaitableTests.cs new file mode 100644 index 000000000000..18ec1d5c8527 --- /dev/null +++ b/src/SignalR/clients/csharp/Client/test/UnitTests/TimerAwaitableTests.cs @@ -0,0 +1,66 @@ +using System; +using System.Runtime.CompilerServices; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Internal; +using Xunit; + +namespace Microsoft.AspNetCore.SignalR.Client.Tests +{ + public class TimerAwaitableTests + { + [Fact] + public void FinalizerRunsIfTimerAwaitableReferencesObject() + { + var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + UseTimerAwaitableAndUnref(tcs); + + // Make sure it *really* cleans up + for (int i = 0; i < 5; i++) + { + GC.Collect(); + GC.WaitForPendingFinalizers(); + } + + // Make sure the finalizer runs + Assert.True(tcs.Task.IsCompleted); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private void UseTimerAwaitableAndUnref(TaskCompletionSource tcs) + { + _ = new ObjectWithTimerAwaitable(tcs).Start(); + } + } + + // This object holds onto a TimerAwaitable referencing the callback (the async continuation is the callback) + // it also has a finalizer that triggers a tcs to callers can be notified when this object is being cleaned up. + public class ObjectWithTimerAwaitable + { + private readonly TimerAwaitable _timer; + private readonly TaskCompletionSource _tcs; + private int _count; + + public ObjectWithTimerAwaitable(TaskCompletionSource tcs) + { + _tcs = tcs; + _timer = new TimerAwaitable(TimeSpan.Zero, TimeSpan.FromSeconds(1)); + _timer.Start(); + } + + public async Task Start() + { + using (_timer) + { + while (await _timer) + { + _count++; + } + } + } + + ~ObjectWithTimerAwaitable() + { + _tcs.TrySetResult(null); + } + } +} diff --git a/src/SignalR/common/Shared/TimerAwaitable.cs b/src/SignalR/common/Shared/TimerAwaitable.cs index ee6cb0fd23c6..c40d55b42d4e 100644 --- a/src/SignalR/common/Shared/TimerAwaitable.cs +++ b/src/SignalR/common/Shared/TimerAwaitable.cs @@ -50,7 +50,20 @@ public void Start() restoreFlow = true; } - _timer = new Timer(state => ((TimerAwaitable)state).Tick(), this, _dueTime, _period); + // This fixes the cycle by using a WeakReference to the state object. The object graph now looks like this: + // Timer -> TimerHolder -> TimerQueueTimer -> WeakReference -> Timer -> ... + // If TimerAwaitable falls out of scope, the timer should be released. + _timer = new Timer(state => + { + var weakRef = (WeakReference)state; + if (weakRef.TryGetTarget(out var thisRef)) + { + thisRef.Tick(); + } + }, + new WeakReference(this), + _dueTime, + _period); } finally { @@ -125,4 +138,4 @@ void IDisposable.Dispose() } } } -} \ No newline at end of file +} From 755609df8ee32680348aec3d65a8cda1c29773ef Mon Sep 17 00:00:00 2001 From: David Fowler Date: Fri, 17 Apr 2020 10:40:52 -0700 Subject: [PATCH 2/2] PR feedback --- .../csharp/Client/test/UnitTests/TimerAwaitableTests.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/SignalR/clients/csharp/Client/test/UnitTests/TimerAwaitableTests.cs b/src/SignalR/clients/csharp/Client/test/UnitTests/TimerAwaitableTests.cs index 18ec1d5c8527..2c005ebe0fe1 100644 --- a/src/SignalR/clients/csharp/Client/test/UnitTests/TimerAwaitableTests.cs +++ b/src/SignalR/clients/csharp/Client/test/UnitTests/TimerAwaitableTests.cs @@ -1,3 +1,6 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + using System; using System.Runtime.CompilerServices; using System.Threading.Tasks; @@ -15,7 +18,7 @@ public void FinalizerRunsIfTimerAwaitableReferencesObject() UseTimerAwaitableAndUnref(tcs); // Make sure it *really* cleans up - for (int i = 0; i < 5; i++) + for (int i = 0; i < 5 && !tcs.Task.IsCompleted; i++) { GC.Collect(); GC.WaitForPendingFinalizers(); @@ -33,7 +36,7 @@ private void UseTimerAwaitableAndUnref(TaskCompletionSource tcs) } // This object holds onto a TimerAwaitable referencing the callback (the async continuation is the callback) - // it also has a finalizer that triggers a tcs to callers can be notified when this object is being cleaned up. + // it also has a finalizer that triggers a tcs so callers can be notified when this object is being cleaned up. public class ObjectWithTimerAwaitable { private readonly TimerAwaitable _timer;