thomaslevesque / Extensions.Hosting.AsyncInitialization

Async initialization for .NET 6.0+ apps using the generic host (e.g. ASP.NET Core apps)
Apache License 2.0
47 stars 7 forks source link

TeardownAsync and Host.DisposeAsync race #18

Open kvpt opened 7 months ago

kvpt commented 7 months ago

Hi,

I also encounter the issue mentioned previously in #16 when using InitAndRunAsync method.

I confirm that under certain conditions there can be a race between TeardownAsync and DisposeAsync on the Host, the parallel stacks bellow illustrate this fact. Capture d'écran 2024-04-03 231550

The InitAndRunAsync method call WaitForShutdownAsync to ensure that the Teardown happen after the shutdown. But there is no guarantee that the host has not been disposed between the WaitForShutdownAsync ending and the call to TeardownAsync method.

The StopAsync method of the Host call the StopAsync method of each registered HostedService and the application Lifecycle. This ensure that each HostedService and Lifecycle has stopped correctly when StopAsync call end. But as this library doesn't use HostedService mecanism it doesn't prevent the call to dispose before the Teardown has executed.

For me it doesn't happen in conventional environment but 95% of the time when I run my integration tests (that use Xunit and Alba) in debug mode.

I tried to reproduce the case in the unit tests of the solution. I not have found a proper way to trigger the problem without adding a hard coded delay to ensure that the dispose always win the race against WaitForShutdownAsync.

To reproduce the issue :

Adding a delay before the teardown inside the InitAndRunAsync method.

...

finally
{
    cts.CancelAfter(teardownTimeout);

    await Task.Delay(TimeSpan.FromSeconds(1), cts.Token);

    await host.TeardownAsync(cts.Token).WaitAsync(cts.Token).ConfigureAwait(false);
}

Run this test:

 [Fact]
 public async Task Teardown_Run_Before_Host_Shutdown()
 {
     var initializer = A.Fake<IAsyncTeardown>();

     var host = CreateHost(services =>
     {
         services.AddAsyncInitializer(initializer);
     });

     IHostApplicationLifetime lifetime = host.Services.GetRequiredService<IHostApplicationLifetime>();

     TaskCompletionSource appStartedTcs = new TaskCompletionSource();

     lifetime.ApplicationStarted.Register(appStartedTcs.SetResult);

     _ = host.InitAndRunAsync();

     await appStartedTcs.Task;

     await host.StopAsync();

     host.Dispose();

     await Task.Delay(TimeSpan.FromSeconds(2));

     A.CallTo(() => initializer.InitializeAsync(A<CancellationToken>._)).MustHaveHappenedOnceExactly();
     A.CallTo(() => initializer.TeardownAsync(A<CancellationToken>._)).MustHaveHappenedOnceExactly();

     Assert.Throws<ObjectDisposedException>(host.Services.CreateScope);
 }

Implementing a HostedService to call TeardownAsync would likely solve the issue but I'm not sure if it a practical solution here because it will prevent the ability to call TeardownAsync when wanted.

I also wonder if it is possible to prevent calling the teardown method when after the init is done we notice that there is no AsyncInitializer that implement IAsyncTeardown. In my case and in #16 also, we don't implement IAsyncTeardown at all.

For now my workaround is to fallback to the old way as I don't use teardown.


await host.InitAsync();
await host.RunAsync();
thomaslevesque commented 7 months ago

Hi @kvpt, Thanks for the detailed report! That's probably not going to be an easy fix 😬 I'll try to look at it in more details tonight.

thomaslevesque commented 7 months ago

OK, so as far as I can tell, there's no way to prevent the host from being disposed once the application is stopped. It's a bit strange that it seems to happen non-deterministically (I would have expected that it would happen either before the call to WaitForShutdownAsync completes, or when explicitly disposed).

I thought we could to take advantage of IHostApplicationLifetime.ApplicationStopping. The docs say:

Triggered when the application host is starting a graceful shutdown.
Shutdown will block until all callbacks registered on this token have completed.

This seemed ideal, except that the cancellation token's callbacks are synchronous, but the teardown is async, so it doesn't really help.

Using a hosted service is an interesting idea. However, we have no control over the order in which the hosted services are stopped. If the StopAsync of another hosted service requires something that is going to be torn down, we need to ensure the teardown hosted service is stopped last... In fact, they might even be stopped in parallel.

Tagging @AleRoe who introduced the teardown feature and might be interested in this.

thomaslevesque commented 7 months ago

In fact, they might even be stopped in parallel.

It is indeed the case if HostOptions.ServicesStopConcurrently is true. So, it depends, and we have no control over it.

Looking at the code for Host.StopAsync, I see there's another hook we could use: IHostedLifecycleService. We could do the teardown in the StoppedAsync method, which runs after all hosted services have been stopped. The only problem that remains is that there might be multiple registered IHostedLifecycleServices, and we need to run last to make sure the teardown doesn't negatively impact the others. And for that, we need to register our IHostedLifecycleService first, so that it's called last during shutdown. Which we can probably do using IServiceCollection.Insert. Of course, someone else could insert their implementation before ours afterwards, but I'd say it's an acceptable limitation.

thomaslevesque commented 7 months ago

Actually, that won't help, at least for older .NET versions. IHostedLifecycleService was introduced in .NET 8, and I'm not willing to drop support for older versions just yet...

thomaslevesque commented 7 months ago

Implementing a HostedService to call TeardownAsync would likely solve the issue but I'm not sure if it a practical solution here because it will prevent the ability to call TeardownAsync when wanted.

I think it would be an acceptable limitation. Calling TeardownAsync manually is tricky anyway, as demonstrated by the fact the solution mentioned in the README doesn't actually work...