search

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.37k stars 9.99k forks source link

Microsoft.Extensions.ApiDescription.Server does not dispose the created host thus could hang/fail the build process #43395

Open bachratyg opened 2 years ago

bachratyg commented 2 years ago

Is there an existing issue for this?

Describe the bug

ApiDescription.Server constructs a service provider by running parts of the application (up to building the IHost), then uses this service provider to retrieve API descriptions. However this host and/or service provider is not disposed properly. This is problematic for services that allocate some background resources and are realized as part of building/configuring the host, typically a logger provider.

Expected Behavior

Generating the OpenAPI doc should not hang or fail.

Steps To Reproduce

The following example creates a logger provider that starts a worker thread. This is similar to what ConsoleLoggerProvider does in https://github.com/dotnet/runtime/tree/v6.0.8/src/libraries/Microsoft.Extensions.Logging.Console/src/ConsoleLoggerProcessor.cs#L30. In this example the thread is not marked as a background thread in order to enforce graceful shutdown.

a.csproj ```csproj net6.0 enable enable true ```
Program.cs ```cs using Microsoft.Extensions.DependencyInjection.Extensions; var builder = WebApplication.CreateBuilder(args); // An instance of this is created when an ILogger is injected to the IHost on .Build() builder.Logging.Services.TryAddEnumerable(ServiceDescriptor.Singleton()); builder.Services.AddEndpointsApiExplorer(); builder.Services.AddSwaggerGen(); var app = builder.Build(); app.Run(); class DisposeMe : ILoggerProvider { private readonly Thread _worker; private readonly CancellationTokenSource _workerStop = new(); public DisposeMe() { Console.WriteLine("I have been constructed"); _worker = new Thread(() => _workerStop.Token.WaitHandle.WaitOne()); // Uncomment for ungraceful shutdown. ConsoleLoggerProvider does this. // _worker.IsBackground = true; _worker.Start(); } public void Dispose() { // Flush cache, etc Console.WriteLine("I have been disposed"); _workerStop.Cancel(); _worker.Join(); } public ILogger CreateLogger(string categoryName) => Microsoft.Extensions.Logging.Abstractions.NullLogger.Instance; } ```

Run dotnet build. After emitting the openapi doc the build process will hang for 2 minutes then fail.

ApiDescriptions.Server constructs a partial application by throwing after the host is built: https://github.com/dotnet/runtime/blob/v6.0.8/src/libraries/Microsoft.Extensions.HostFactoryResolver/src/HostFactoryResolver.cs#L344 This exception is swallowed at https://github.com/dotnet/runtime/blob/55fb7ef977e7d120dc12f0960edcff0739d7ee0e/src/libraries/Microsoft.Extensions.HostFactoryResolver/src/HostFactoryResolver.cs#L252 The process then fails to quit because the worker thread is still running.

If running the app directly the app.Run() call would dispose the host or an exception would crash the entire process.

Exceptions (if any)

(removed some clutter)

Microsoft.Extensions.ApiDescription.Server.targets(66,5): error :
System.TimeoutException: Process C:\Program Files\dotnet\dotnet.exe timed out after 2 minutes.
at Microsoft.Extensions.ApiDescription.Tool.Exe.Run(String executable, IReadOnlyList`1 args, IReporter reporter, String workingDirectory, Boolean interceptOutput)
at Microsoft.Extensions.ApiDescription.Tool.Commands.InvokeCommand.Execute()
at Microsoft.Extensions.ApiDescription.Tool.Commands.CommandBase.<>c__DisplayClass14_0.<Configure>b__0()
at Microsoft.Extensions.CommandLineUtils.CommandLineApplication.Execute(String[]args)
at Microsoft.Extensions.ApiDescription.Tool.ProgramBase.Run(String[] args, CommandBase command, Boolean throwOnUnexpectedArg)

Microsoft.Extensions.ApiDescription.Server.targets(66,5): error MSB3073:
The command "dotnet "****\../tools/dotnet-getdocument.dll" --assembly "***\bin\Debug\net6.0\a.dll" --file-list "obj\a.OpenApiFiles.cache" --framework ".NETCoreApp,Version=v6.0" --output "obj" --project "a" --assets-file "****\obj\project.assets.json" --platform "AnyCPU" " exited with code 1.

.NET Version

6.0.400

Anything else?

Some related issues:

14410 assumptions about the user code that runs as part of the API discovery are still not documented

23033 some current configuration APIs simply don't support deferring actual work past the host startup.

43391 the host is intercepted before all user code that affects the API had a chance to run

Some considerations on how to resolve this

1. Don't run user code. Ever. Ideally no user code should be run as part of the build process. This would be bordering on the impossible with the current API shape.

2. Allow the host to be fully constructed and disposed by user code A possible solution (that would also solve #43391) is to capture the host right before starting it. This would allow using this pattern in user code to dispose the host:

3. Pass a different environment The generator should pass --environment GeneratingApi or something similar to the app when generating the API doc so that user code could trim unnecessary/dangerous services and configuration. Maybe even make the host refuse to start or immediately quit when this parameter is present. This is probably the easiest to implement.

await using var app = builder.Build(); // Currently this would throw an exception before assigning to the variable
// ...
await app.StartAsync();
await app.WaitForShutdownAsync();

However this would have the drawback that even more services with side effects could be realized when configuring the pipeline.

dotnet --info ``` .NET SDK (reflecting any global.json): Version: 6.0.400 Commit: 7771abd614 Runtime Environment: OS Name: Windows OS Version: 10.0.19044 OS Platform: Windows RID: win10-x64 Base Path: C:\Program Files\dotnet\sdk\6.0.400\ Host: Version: 7.0.0-preview.7.22375.6 Architecture: x64 Commit: eecb028078 .NET SDKs installed: 6.0.400 [C:\Program Files\dotnet\sdk] 7.0.100-preview.7.22377.5 [C:\Program Files\dotnet\sdk] .NET runtimes installed: Microsoft.AspNetCore.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 6.0.8 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 7.0.0-preview.7.22376.6 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.NETCore.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 6.0.8 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 7.0.0-preview.7.22375.6 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.WindowsDesktop.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 6.0.8 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 7.0.0-preview.7.22377.1 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Other architectures found: arm64 [C:\Program Files\dotnet] registered at [HKLM\SOFTWARE\dotnet\Setup\InstalledVersions\arm64\InstallLocation] x86 [C:\Program Files (x86)\dotnet] registered at [HKLM\SOFTWARE\dotnet\Setup\InstalledVersions\x86\InstallLocation] Environment variables: Not set ```
davidfowl commented 2 years ago

The console logging thread won't hang the app because it's a background thread but your repro will because it isn't.

Ideally no user code should be run as part of the build process. This would be bordering on the impossible with the current API shape.

This is a non-starter.

  1. Allow the host to be fully constructed and disposed by user code

See my comment here https://github.com/dotnet/aspnetcore/issues/43391#issuecomment-1220836423

  1. Pass a different environment

This is the one that makes the most sense and is cooperative. We should also provide an API that can be used to detect this mode so that the app developer can avoid side effects etc.

bachratyg commented 2 years ago

The console logging thread won't hang the app because it's a background thread but your repro will because it isn't.

Like I said

in order to enforce graceful shutdown // Uncomment for ungraceful shutdown. ConsoleLoggerProvider does this.

I might have been overly cautious with this. The point is if the live app is terminating in an unexpected way for whatever reason then having the process hang around a bit longer (or possibly indefinitely) might be the better option than losing potential log messages that show the way to the crash.

ghost commented 2 years ago

Thanks for contacting us. We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

captainsafia commented 2 years ago

During triage, we talked about whether or not the fix to support the minimal host might've resolved this. I didn't think it did but validated the hunch now that we have a release with the working ApiDescription.Server. Alas, it doesn't. I'll leave this in .NET 8 Planning for now. I think this is probably roughly a ~P2.