dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.3k stars 9.96k forks source link

Reduce chance of stopping a WebApplication incorrectly by providing compiler info/warning #50578

Open mconnew opened 1 year ago

mconnew commented 1 year ago

Background and Motivation

There are two steps to shutting down a WebApplication, calling StopAsync, and disposing it. Omitting either of these steps can have negative consequences. If someone is transitioning from WebHost to WebApplication, the dispose behavior is different and can lead to introducing a bug/unwanted behavior. If you don't call WebHost.StopAsync, when you dispose the WebHost it implicitly calls StopAsync. WebApplication does not ensure StopAsync has been called when you dispose it. The consequences of this is that any IHostedService instances won't have their StopAsync method called when disposing a WebApplication whereas when using WebHost they did.
The second problem is if you omit disposing your WebApplication, you will leak some memory which is never reclaimed by GC. See #45621 for details.

Proposed Analyzer

Analyzer Behavior and Message

If a WebApplication is used with a using statement and there is not a call to StopAsync within the using statement code block, then an info message should be emitted informing the developer that any IHostedService instances may not be cleanly stopped if disposing without calling StopAsync. Similarly if not using a using block but manually calling Dispose/DisposeAsync, this is also the case. This would be an info usage rule.

If a call to WebApplication.StopAsync is made without a subsequent call to WebApplication.Dispose() or WebApplication.DisposeAsync(), then a warning should be emitted telling the developer that they will leak memory. Alternatively, fix issue #45621 and then this won't be needed, but there doesn't seem to be any traction on that issue. This would be a warning reliability rule.

Category

Severity Level

Usage Scenarios

This should trigger the info about needing to call StopAsync:

var builder = WebApplication.CreateBuilder(Array.Empty<string>());
builder.WebHost.UseKestrel(options =>
{
    options.ListenLocalhost(8443);
});
builder.Logging.ClearProviders();
await using (var app = builder.Build())
{
    app.Run(async context =>
    {
        await context.Response.WriteAsync("Hello world!");
    });
    await app.StartAsync();
    await SomeExternalWaitingMechanismAsync();
}

This code should also trigger the info about needing to call StopAsync:

var builder = WebApplication.CreateBuilder(Array.Empty<string>());
builder.WebHost.UseKestrel(options =>
{
    options.ListenLocalhost(8443);
});
builder.Logging.ClearProviders();
var app = builder.Build();
app.Run(async context =>
{
    await context.Response.WriteAsync("Hello world!");
});
await app.StartAsync();
await SomeExternalWaitingMechanismAsync();

Followed by one of the following lines:

((IDisposeable)app).Dispose(); // WebApplication explicitly implements IDisposable so must be cast
(app as IDisposable).Dispose();
(app as IDisposable)?.Dispose(); // null propagation as compiler can't tell whether app implements IDisposable
await app.DisposeAsync();

This should trigger the warning about needing to dispose the WebApplication

var builder = WebApplication.CreateBuilder(Array.Empty<string>());
builder.WebHost.UseKestrel(options =>
{
    options.ListenLocalhost(8443);
});
builder.Logging.ClearProviders();
var app = builder.Build();
app.Run(async context =>
{
    await context.Response.WriteAsync("Hello world!");
});
await app.StartAsync();
await SomeExternalWaitingMechanismAsync();
await app.StopAsync();

Risks

The overhead of running the analyzer will take a non-zero amount of time.

ghost commented 11 months ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.