Make BackgroundService run ExecuteAsync as task (#116283)

* Make BackgroundService run ExecuteAsync as task

* Simplify BackGroundService.StartAsync, fix tests

* Never return ExecuteAsync task from StartAsync

It's possible the ExecuteAsync task completes in the time between it was
started and StartAsync returns.  If so, we were returning it's task.
This leads to non-deterministic error behavior of the StartAsync API as
well as inconsistently honoring `BackgroundServiceExceptionBehavior`
based on this race.

Instead always return a completed task.

* Address feedback

* Address feedback

* Expose cancellation async

* Update src/libraries/Microsoft.Extensions.Hosting.Abstractions/src/BackgroundService.cs

Co-authored-by: Stephen Toub <[email protected]>

---------

Co-authored-by: Stephen Toub <[email protected]>

Author: ericstj

src/libraries/Microsoft.Extensions.Hosting.Abstractions/src/BackgroundService.cs

@@ -42,16 +42,10 @@ public virtual Task StartAsync(CancellationToken cancellationToken)
             // Create linked token to allow cancelling executing task from provided token
             _stoppingCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
 
-            // Store the task we're executing
-            _executeTask = ExecuteAsync(_stoppingCts.Token);
+            // Execute all of ExecuteAsync asynchronously, and store the task we're executing so that we can wait for it later.
+            _executeTask = Task.Run(() => ExecuteAsync(_stoppingCts.Token), _stoppingCts.Token);
 
-            // If the task is completed then return it, this will bubble cancellation and failure to the caller
-            if (_executeTask.IsCompleted)
-            {
-                return _executeTask;
-            }
-
-            // Otherwise it's running
+            // Always return a completed task.  Any result from ExecuteAsync will be handled by the Host.
             return Task.CompletedTask;
         }
 

src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/BackgroundServiceTests.cs

@@ -11,7 +11,7 @@ namespace Microsoft.Extensions.Hosting.Tests
     public class BackgroundServiceTests
     {
         [Fact]
-        public void StartReturnsCompletedTaskIfLongRunningTaskIsIncomplete()
+        public void StartReturnsCompletedTask()
         {
             var tcs = new TaskCompletionSource<object>();
             var service = new MyBackgroundService(tcs.Task);
@@ -26,28 +26,14 @@ public void StartReturnsCompletedTaskIfLongRunningTaskIsIncomplete()
         }
 
         [Fact]
-        public void StartReturnsCompletedTaskIfCancelled()
+        public async Task StartCancelledThrowsTaskCanceledException()
         {
-            var tcs = new TaskCompletionSource<object>();
-            tcs.TrySetCanceled();
-            var service = new MyBackgroundService(tcs.Task);
-
-            var task = service.StartAsync(CancellationToken.None);
-
-            Assert.True(task.IsCompleted);
-            Assert.Same(task, service.ExecuteTask);
-        }
-
-        [Fact]
-        public async Task StartReturnsLongRunningTaskIfFailed()
-        {
-            var tcs = new TaskCompletionSource<object>();
-            tcs.TrySetException(new Exception("fail!"));
-            var service = new MyBackgroundService(tcs.Task);
+            var ct = new CancellationToken(true);
+            var service = new WaitForCancelledTokenService();
 
-            var exception = await Assert.ThrowsAsync<Exception>(() => service.StartAsync(CancellationToken.None));
+            await service.StartAsync(ct);
 
-            Assert.Equal("fail!", exception.Message);
+            await Assert.ThrowsAnyAsync<OperationCanceledException>(() => service.ExecuteTask);
         }
 
         [Fact]
@@ -91,6 +77,7 @@ public async Task StopAsyncThrowsIfCancellationCallbackThrows()
             var service = new ThrowOnCancellationService();
 
             await service.StartAsync(CancellationToken.None);
+            await service.WaitForExecuteTask;
 
             var cts = new CancellationTokenSource(TimeSpan.FromSeconds(1));
             await Assert.ThrowsAsync<AggregateException>(() => service.StopAsync(cts.Token));
@@ -116,6 +103,7 @@ public async Task StartAsyncThenCancelShouldCancelExecutingTask()
             var service = new WaitForCancelledTokenService();
 
             await service.StartAsync(tokenSource.Token);
+            await service.WaitForExecuteTask;
 
             tokenSource.Cancel();
 
@@ -130,19 +118,58 @@ public void CreateAndDisposeShouldNotThrow()
             service.Dispose();
         }
 
+        [Fact]
+        public async Task StartSynchronousAndStop()
+        {
+            var tokenSource = new CancellationTokenSource();
+            var service = new MySynchronousBackgroundService();
+
+            // should not block the start thread;
+            await service.StartAsync(tokenSource.Token);
+            await service.WaitForExecuteTask;
+            await service.StopAsync(CancellationToken.None);
+
+            Assert.True(service.WaitForEndExecuteTask.IsCompleted);
+        }
+
+        [Fact]
+        public async Task StartSynchronousExecuteShouldBeCancelable()
+        {
+            var tokenSource = new CancellationTokenSource();
+            var service = new MySynchronousBackgroundService();
+
+            await service.StartAsync(tokenSource.Token);
+            await service.WaitForExecuteTask;
+
+            tokenSource.Cancel();
+
+            await service.WaitForEndExecuteTask;
+        }
+
         private class WaitForCancelledTokenService : BackgroundService
         {
+            private TaskCompletionSource<object> _waitForExecuteTask = new TaskCompletionSource<object>();
+
             public Task ExecutingTask { get; private set; }
 
+            public Task WaitForExecuteTask => _waitForExecuteTask.Task;
+
             protected override Task ExecuteAsync(CancellationToken stoppingToken)
             {
                 ExecutingTask = Task.Delay(Timeout.Infinite, stoppingToken);
+
+                _waitForExecuteTask.TrySetResult(null);
+
                 return ExecutingTask;
             }
         }
 
         private class ThrowOnCancellationService : BackgroundService
         {
+            private TaskCompletionSource<object> _waitForExecuteTask = new TaskCompletionSource<object>();
+
+            public Task WaitForExecuteTask => _waitForExecuteTask.Task;
+
             public int TokenCalls { get; set; }
 
             protected override Task ExecuteAsync(CancellationToken stoppingToken)
@@ -158,6 +185,8 @@ protected override Task ExecuteAsync(CancellationToken stoppingToken)
                     TokenCalls++;
                 });
 
+                _waitForExecuteTask.TrySetResult(null);
+
                 return new TaskCompletionSource<object>().Task;
             }
         }
@@ -191,5 +220,24 @@ private async Task ExecuteCore(CancellationToken stoppingToken)
                 await task;
             }
         }
+
+        private class MySynchronousBackgroundService : BackgroundService
+        {
+            private TaskCompletionSource<object> _waitForExecuteTask = new TaskCompletionSource<object>();
+            private TaskCompletionSource<object> _waitForEndExecuteTask = new TaskCompletionSource<object>();
+
+            public Task WaitForExecuteTask => _waitForExecuteTask.Task;
+            public Task WaitForEndExecuteTask => _waitForEndExecuteTask.Task;
+
+#pragma warning disable CS1998 // Async method lacks 'await' operators and will run synchronously
+            protected override async Task ExecuteAsync(CancellationToken stoppingToken)
+            {
+                _waitForExecuteTask.TrySetResult(null);
+                stoppingToken.WaitHandle.WaitOne();
+                _waitForEndExecuteTask.TrySetResult(null);
+            }
+#pragma warning restore CS1998 // Async method lacks 'await' operators and will run synchronously
+
+        }
     }
 }

src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/Internal/HostTests.cs

@@ -688,7 +688,7 @@ public async Task WebHostStopAsyncUsesDefaultTimeoutIfNoTokenProvided()
         }
 
         [Fact]
-        public async Task HostPropagatesExceptionsThrownWithBackgroundServiceExceptionBehaviorOfStopHost()
+        public async Task HostStopsWithBackgroundServiceExceptionBehaviorOfStopHost()
         {
             using IHost host = CreateBuilder()
                 .ConfigureServices(
@@ -702,7 +702,12 @@ public async Task HostPropagatesExceptionsThrownWithBackgroundServiceExceptionBe
                     })
                 .Build();
 
-            await Assert.ThrowsAsync<Exception>(() => host.StartAsync());
+            await host.StartAsync();
+
+            // host is expected to catch exception, then trigger ApplicationStopping.
+            // give the host 1 minute to stop.
+            var lifetime = host.Services.GetRequiredService<IHostApplicationLifetime>();
+            Assert.True(lifetime.ApplicationStopping.WaitHandle.WaitOne(TimeSpan.FromMinutes(1)));
         }
 
         [Fact]