Consume PosixSignal in Hosting's ConsoleLifetime #56057
Conversation
eerhardt
requested review from
davidfowl,
Tratcher,
maryamariyan and
IEvangelist
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
Tagging subscribers to this area: @eerhardt, @maryamariyan Issue DetailsThis fixes quite a few Hosting exit issues related to ConsoleLifetime. It is probably best to review the first commit by itself - The rest of the commits fix the individual bugs. Fix #55417 cc @stephentoub @jkotas - FYI in case you want to take a look
|
Don't listen to ProcessExit on net6.0+ in Hosting anymore. This allows for Environment.Exit to not hang the app. Don't clobber ExitCode during ProcessExit now that SIGTERM is handled separately. For non-net6.0 targets, only wait for the shutdown timeout, so the process doesn't hang forever. Fix dotnet#55417 Fix dotnet#44086 Fix dotnet#50397 Fix dotnet#42224 Fix dotnet#35990
eerhardt
force-pushed
the
HostingPosixSignal
branch
from
a4574cc to
280e63e
Compare
src/libraries/Microsoft.Extensions.Hosting/src/HostingHostBuilderExtensions.cs
Show resolved
Hide resolved
| </ItemGroup> | ||
|
|
||
| <ItemGroup Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net6.0'))"> | ||
| <Compile Remove="Internal\ConsoleLifetime.notnetcoreapp.cs" /> |
There was a problem hiding this comment.
Suggestion for a better name?
There was a problem hiding this comment.
This library also builds for net461 and that TFM will compile this file. Is netstandard.cs used elsewhere when targeting net461? I can use that name if it makes sense to people.
Aside: I plan on proposing that in .NET 7 we change Microsoft.Extensions.Hosting to only target .NET 6+ going forward. Microsoft.Extensions.Hosting is an app model that should only be referenced by executables, libraries shouldn't reference it. So just like winforms/wpf/aspnetcore/maui/etc, we should only need to target .NET 6+ in Hosting.
src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/ConsoleLifetimeExitTests.cs
Outdated
Show resolved
Hide resolved
|
/azp run runtime |
|
Azure Pipelines successfully started running 1 pipeline(s). |
src/libraries/Microsoft.Extensions.Hosting/src/Internal/ConsoleLifetime.netcoreapp.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Hosting/src/Internal/ConsoleLifetime.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Hosting/src/Internal/ConsoleLifetime.cs
Outdated
Show resolved
Hide resolved
* Remove _shutdownBlock on netcoreappcurrent, as this is no longer waited on * Change Console.CancelKeyPress to use PosixSignalRegistration SIGINT and SIGQUIT * Use OperatingSystem.IsWindows * Use a no-op lifetime on mobile platforms
|
|
||
| namespace Microsoft.Extensions.Hosting.Internal | ||
| { | ||
| public partial class ConsoleLifetime : IHostLifetime |
* Fix broken CI tests
| `Host.Run` in the Main method. | ||
|
|
||
| This caused other issues because SIGTERM wasn't the only way `ProcessExit` was raised. It is also raised by code | ||
| in the application calling `Environment.Exit`. `Environment.Exit` isn't a graceful way of shutting down a process. |
There was a problem hiding this comment.
| in the application calling `Environment.Exit`. `Environment.Exit` isn't a graceful way of shutting down a process. | |
| in the application calling `Environment.Exit`. `Environment.Exit` isn't a graceful way of shutting down a process in the `Microsoft.Extensions.Hosting` app model. |
There was a problem hiding this comment.
Is it a graceful way of shutting down a process in any app model?
There was a problem hiding this comment.
Environment.Exit is a graceful program termination from the runtime point of view.
App models typically want to wrap exit with their own logic. I agree that it is probably not considered the right way to exit the process in any app model, except the "no app model" simple apps.
There was a problem hiding this comment.
I’ll fix this up in a follow up PR tomorrow so I don’t need to reset CI.
ghost
locked as resolved and limited conversation to collaborators
This fixes quite a few Hosting exit issues related to ConsoleLifetime.
It is probably best to review the first commit by itself -
Add NetCoreAppCurrent target to Microsoft.Extensions.Hosting. I needed to add anet6.0target so I could use the new Signal APIs from #50527.The rest of the commits fix the individual bugs.
Fix #55417
Fix #44086
Fix #50397
Fix #42224
Fix #35990
cc @stephentoub @jkotas - FYI in case you want to take a look