From 2f901a105cdd7d1c3c4d52f4a769478d1f4d7db4 Mon Sep 17 00:00:00 2001 From: Brant Burnett Date: Fri, 11 Mar 2022 08:47:35 -0500 Subject: [PATCH 1/3] Ensure continuations run with sync task completion (ARCH-98) Motivation ---------- There is currently a corner case when continuations are queued by a synchronously completing task where those continuations are never executed. Modifications ------------- When the task completes synchronously queue all remaining continuations on the parent synchronization context. Switch build/tests to .NET 6 and language to C# 10. --- .github/workflows/build.yml | 6 +- .../CenterEdge.Async.Benchmarks.csproj | 4 +- .../AsyncHelperTests.cs | 138 ++++++++++++++++++ .../CenterEdge.Async.UnitTests.csproj | 10 +- src/CenterEdge.Async/AsyncHelper.cs | 73 ++++++--- src/CenterEdge.Async/CenterEdge.Async.csproj | 2 +- 6 files changed, 203 insertions(+), 30 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 5f69f72..156e8cd 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -19,7 +19,7 @@ jobs: - name: Install GitVersion uses: gittools/actions/gitversion/setup@v0.9.10 with: - versionSpec: "5.7.0" + versionSpec: "5.8.0" - name: Determine Version id: gitversion @@ -31,7 +31,7 @@ jobs: - name: Setup .NET Core uses: actions/setup-dotnet@v1 with: - dotnet-version: 5.0.x + dotnet-version: 6.0.x # Cache packages for faster subsequent runs - uses: actions/cache@v2 @@ -54,7 +54,7 @@ jobs: - name: Test working-directory: ./src run: dotnet test --no-build -c Release -f net5.0 -l 'trx;LogFileName=${{ runner.temp }}/results.trx' ./CenterEdge.Async.sln - + - name: Test Report uses: dorny/test-reporter@v1 if: success() || failure() # run this step even if previous step failed diff --git a/src/CenterEdge.Async.Benchmarks/CenterEdge.Async.Benchmarks.csproj b/src/CenterEdge.Async.Benchmarks/CenterEdge.Async.Benchmarks.csproj index 1a2e148..dc90130 100644 --- a/src/CenterEdge.Async.Benchmarks/CenterEdge.Async.Benchmarks.csproj +++ b/src/CenterEdge.Async.Benchmarks/CenterEdge.Async.Benchmarks.csproj @@ -1,8 +1,8 @@  - net48;netcoreapp3.1;net5.0 + net48;netcoreapp3.1;net6.0 Exe - 9 + 10 AnyCPU diff --git a/src/CenterEdge.Async.UnitTests/AsyncHelperTests.cs b/src/CenterEdge.Async.UnitTests/AsyncHelperTests.cs index a9c630b..089e84b 100644 --- a/src/CenterEdge.Async.UnitTests/AsyncHelperTests.cs +++ b/src/CenterEdge.Async.UnitTests/AsyncHelperTests.cs @@ -32,6 +32,41 @@ public void RunSync_Task_DoesAllTasks() Assert.Equal(3, i); } + [Fact] + public async Task RunSync_StartsTasksAndCompletesSynchronously_DoesAllTasks() + { + // Replicates the case where continuations are queued but the main task completes synchronously + // so the work must be removed from the queue + + // Arrange + + var i = 0; + + async Task IncrementAsync() + { + await Task.Yield(); + Interlocked.Increment(ref i); + } + + // Act + AsyncHelper.RunSync(() => + { +#pragma warning disable CS4014 + for (var j = 0; j < 3; j++) + { + var _ = IncrementAsync(); + } +#pragma warning restore CS4014 + + return Task.CompletedTask; + }); + + // Assert + + await Task.Delay(500); + Assert.Equal(3, i); + } + [Fact] public void RunSync_Task_ConfigureAwaitFalse_DoesAllTasks() { @@ -157,7 +192,40 @@ public void RunSync_ValueTask_DoesAllTasks() Assert.Equal(3, i); } + [Fact] + public async Task RunSync_ValueTask_StartsTasksAndCompletesSynchronously_DoesAllTasks() + { + // Replicates the case where continuations are queued but the main task completes synchronously + // so the work must be removed from the queue + + // Arrange + + var i = 0; + + async Task IncrementAsync() + { + await Task.Yield(); + Interlocked.Increment(ref i); + } + // Act + AsyncHelper.RunSync(() => + { +#pragma warning disable CS4014 + for (var j = 0; j < 3; j++) + { + var _ = IncrementAsync(); + } +#pragma warning restore CS4014 + + return new ValueTask(); + }); + + // Assert + + await Task.Delay(500); + Assert.Equal(3, i); + } [Fact] public void RunSync_ValueTask_ConfigureAwaitFalse_DoesAllTasks() @@ -281,6 +349,41 @@ public void RunSync_TaskT_DoesAllTasks() Assert.Equal(3, result); } + [Fact] + public async Task RunSync_TaskT_StartsTasksAndCompletesSynchronously_DoesAllTasks() + { + // Replicates the case where continuations are queued but the main task completes synchronously + // so the work must be removed from the queue + + // Arrange + + var i = 0; + + async Task IncrementAsync() + { + await Task.Yield(); + Interlocked.Increment(ref i); + } + + // Act + AsyncHelper.RunSync(() => + { +#pragma warning disable CS4014 + for (var j = 0; j < 3; j++) + { + var _ = IncrementAsync(); + } +#pragma warning restore CS4014 + + return Task.FromResult(true); + }); + + // Assert + + await Task.Delay(500); + Assert.Equal(3, i); + } + [Fact] public void RunSync_TaskT_ConfigureAwaitFalse_DoesAllTasks() { @@ -402,6 +505,41 @@ public void RunSync_ValueTaskT_DoesAllTasks() Assert.Equal(3, result); } + [Fact] + public async Task RunSync_ValueTaskT_StartsTasksAndCompletesSynchronously_DoesAllTasks() + { + // Replicates the case where continuations are queued but the main task completes synchronously + // so the work must be removed from the queue + + // Arrange + + var i = 0; + + async Task IncrementAsync() + { + await Task.Yield(); + Interlocked.Increment(ref i); + } + + // Act + AsyncHelper.RunSync(() => + { +#pragma warning disable CS4014 + for (var j = 0; j < 3; j++) + { + var _ = IncrementAsync(); + } +#pragma warning restore CS4014 + + return new ValueTask(true); + }); + + // Assert + + await Task.Delay(500); + Assert.Equal(3, i); + } + [Fact] public void RunSync_ValueTaskT_ConfigureAwaitFalse_DoesAllTasks() { diff --git a/src/CenterEdge.Async.UnitTests/CenterEdge.Async.UnitTests.csproj b/src/CenterEdge.Async.UnitTests/CenterEdge.Async.UnitTests.csproj index e16133e..34fe6e9 100644 --- a/src/CenterEdge.Async.UnitTests/CenterEdge.Async.UnitTests.csproj +++ b/src/CenterEdge.Async.UnitTests/CenterEdge.Async.UnitTests.csproj @@ -1,26 +1,26 @@ - net48;net5.0 + net48;net6.0 - 9 + 10 warnings false - + runtime; build; native; contentfiles; analyzers; buildtransitive all - + all runtime; build; native; contentfiles; analyzers; buildtransitive - + all diff --git a/src/CenterEdge.Async/AsyncHelper.cs b/src/CenterEdge.Async/AsyncHelper.cs index d6bb400..f0dfb0e 100644 --- a/src/CenterEdge.Async/AsyncHelper.cs +++ b/src/CenterEdge.Async/AsyncHelper.cs @@ -37,6 +37,10 @@ public static void RunSync(Func task) { synch.Run(awaiter); } + else + { + synch.RunAlreadyComplete(); + } // Throw any exception returned by the task awaiter.GetResult(); @@ -68,6 +72,10 @@ public static void RunSync(Func task) { synch.Run(awaiter); } + else + { + synch.RunAlreadyComplete(); + } // Throw any exception returned by the task awaiter.GetResult(); @@ -100,6 +108,10 @@ public static T RunSync(Func> task) { synch.Run(awaiter); } + else + { + synch.RunAlreadyComplete(); + } // Throw any exception returned by the task or return the result return awaiter.GetResult(); @@ -132,6 +144,10 @@ public static T RunSync(Func> task) { synch.Run(awaiter); } + else + { + synch.RunAlreadyComplete(); + } // Throw any exception returned by the task or return the result return awaiter.GetResult(); @@ -178,25 +194,7 @@ public override void Post(SendOrPostCallback d, object? state) // This can occur if the main task starts additional work which isn't completed // before the main task completes. - if (_parentSynchronizationContext != null) - { - _parentSynchronizationContext.Post(d, state); - } - else - { - // There is no parent sync context, so use the default behavior from the default - // SynchronizationContext and post to the thread pool. - - #if NETSTANDARD2_1_OR_GREATER || NET5_0_OR_GREATER - ThreadPool.QueueUserWorkItem(static s => s.d(s.state), (d, state), preferLocal: false); - #else - ThreadPool.QueueUserWorkItem(static s => - { - var state = ((SendOrPostCallback d, object? state))s; - state.d(state.state); - }, (d, state)); - #endif - } + ExecuteOnParent(d, state); } private void EndMessageLoop() @@ -228,6 +226,43 @@ public void Run(TAwaiter awaiter) } } + // Processes any remaining continuations in the queue + public void RunAlreadyComplete() + { + EndMessageLoop(); + + while (!_items.IsCompleted) + { + var task = _items.Take(); + + ExecuteOnParent(task.Callback, task.State); + } + } + + // Executes a work item on the parent SynchronizationContext or on the thread pool if there is not one + private void ExecuteOnParent(SendOrPostCallback callback, object? state) + { + if (_parentSynchronizationContext != null) + { + _parentSynchronizationContext.Post(callback, state); + } + else + { + // There is no parent sync context, so use the default behavior from the default + // SynchronizationContext and post to the thread pool. + +#if NETSTANDARD2_1_OR_GREATER || NET5_0_OR_GREATER + ThreadPool.QueueUserWorkItem(static s => s.callback(s.state), (callback, state), preferLocal: false); +#else + ThreadPool.QueueUserWorkItem(static s => + { + var state = ((SendOrPostCallback callback, object? state))s; + state.callback(state.state); + }, (callback, state)); +#endif + } + } + public override SynchronizationContext CreateCopy() { return this; diff --git a/src/CenterEdge.Async/CenterEdge.Async.csproj b/src/CenterEdge.Async/CenterEdge.Async.csproj index 397e787..3da05da 100644 --- a/src/CenterEdge.Async/CenterEdge.Async.csproj +++ b/src/CenterEdge.Async/CenterEdge.Async.csproj @@ -3,7 +3,7 @@ netstandard2.0;netstandard2.1;net5.0 - 9 + 10 enable true true From 45b8d4b81a74753c921fd0298b8d332f69494ec3 Mon Sep 17 00:00:00 2001 From: Brant Burnett Date: Fri, 11 Mar 2022 08:53:15 -0500 Subject: [PATCH 2/3] actions tweak --- .github/workflows/build.yml | 4 ++-- .github/workflows/cleanup-packages.yml | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) create mode 100644 .github/workflows/cleanup-packages.yml diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 156e8cd..a781c1d 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -53,14 +53,14 @@ jobs: - name: Test working-directory: ./src - run: dotnet test --no-build -c Release -f net5.0 -l 'trx;LogFileName=${{ runner.temp }}/results.trx' ./CenterEdge.Async.sln + run: dotnet test --no-build -c Release -f net5.0 -l 'trx;LogFileName=results.trx' ./CenterEdge.Async.sln - name: Test Report uses: dorny/test-reporter@v1 if: success() || failure() # run this step even if previous step failed with: name: Unit Tests - path: ${{ runner.temp }}/results.trx + path: src/**/results.trx reporter: dotnet-trx - name: Pack diff --git a/.github/workflows/cleanup-packages.yml b/.github/workflows/cleanup-packages.yml new file mode 100644 index 0000000..9656b08 --- /dev/null +++ b/.github/workflows/cleanup-packages.yml @@ -0,0 +1,20 @@ +name: Cleanup packages + +on: + schedule: + - cron: '21 0 * * 6' + workflow_dispatch: + +jobs: + clean-pr-packages: + + runs-on: ubuntu-latest + permissions: + packages: write + + steps: + - uses: actions/delete-package-versions@v2 + with: + package-name: CenterEdge.Async + min-versions-to-keep: 30 + ignore-versions: ^(?!.*ci-pr).*$ From 452236b645494f0168d8079de1eb164c5a6fa0e8 Mon Sep 17 00:00:00 2001 From: Brant Burnett Date: Fri, 11 Mar 2022 08:57:42 -0500 Subject: [PATCH 3/3] oops --- .github/workflows/build.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index a781c1d..0fe50be 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -53,7 +53,7 @@ jobs: - name: Test working-directory: ./src - run: dotnet test --no-build -c Release -f net5.0 -l 'trx;LogFileName=results.trx' ./CenterEdge.Async.sln + run: dotnet test --no-build -c Release -f net6.0 -l 'trx;LogFileName=results.trx' ./CenterEdge.Async.sln - name: Test Report uses: dorny/test-reporter@v1