Handle SIGTERMs for graceful shutdown #876
Conversation
pakrym
force-pushed
the
pakrym/SIGTERM
branch
3 times, most recently
from
436f735 to
c6400a1
Compare
| @@ -16,18 +19,26 @@ public static class WebHostExtensions | |||
| /// <param name="host">The <see cref="IWebHost"/> to run.</param> | |||
| public static void Run(this IWebHost host) | |||
| { | |||
| AutoResetEvent done = new AutoResetEvent(false); | |||
| @@ -30,7 +30,7 @@ | |||
| "System.Net.Http": "" | |||
| } | |||
| }, | |||
| "netstandard1.3": { | |||
| "netstandard1.5": { | |||
There was a problem hiding this comment.
This is a pretty big deal. Can't this also target both?
|
|
||
| // Don't terminate the process immediately, wait for the Main thread to exit gracefully. | ||
| eventArgs.Cancel = true; |
There was a problem hiding this comment.
How big of a change is this from the default behavior? Any observable differences?
There was a problem hiding this comment.
If there is something in Program.Main after host.Run it would not get run. But at least it would be consistent between SIGTERM and ctrl+c
There was a problem hiding this comment.
That's a big one... We need to think about that...
pakrym
force-pushed
the
pakrym/SIGTERM
branch
2 times, most recently
from
185c48f to
f235a10
Compare
| @@ -5,6 +5,9 @@ | |||
| using System.Threading; | |||
| using Microsoft.AspNetCore.Hosting.Server.Features; | |||
| using Microsoft.Extensions.DependencyInjection; | |||
| #if NETSTANDARD1_5 | |||
| using System.Runtime.Loader; | |||
|
|
||
| // Don't terminate the process immediately, wait for the Main thread to exit gracefully. | ||
| eventArgs.Cancel = true; | ||
| done.WaitOne(); |
There was a problem hiding this comment.
Not running client code after Run is a major regression.
There was a problem hiding this comment.
There is no way we can support this with SIGTERM. So we need to decide if we want to have two behaviors or one common.
There was a problem hiding this comment.
Sounds like we need better CLR events for SIGTERM.
| @@ -34,6 +34,11 @@ | |||
| "dependencies": { | |||
| "System.Diagnostics.Contracts": "4.3.0-*" | |||
| } | |||
| }, | |||
| "netstandard1.5": { | |||
There was a problem hiding this comment.
why did TestHost need to change? Shouldn't it have been fine building against the 1.3 version of Hosting?
pakrym
force-pushed
the
pakrym/SIGTERM
branch
2 times, most recently
from
8dfafda to
4390e0e
Compare
|
Fixed comments, brought old behavior for |
| }; | ||
|
|
||
| #if NETSTANDARD1_5 | ||
| AssemblyLoadContext.Default.Unloading += context => shutdown(); |
There was a problem hiding this comment.
So 80% of the case will be using this in program.Main(), for the 20% case though you might want to use the load context associated with this assembly instance.
var alc = AssemblyLoadContext.GetLoadContext(typeof(WebHostExtensions).GetTypeInfo().Assembly);
alc.Unloading += context => shutdown();
| @@ -16,18 +19,32 @@ public static class WebHostExtensions | |||
| /// <param name="host">The <see cref="IWebHost"/> to run.</param> | |||
| public static void Run(this IWebHost host) | |||
| { | |||
| var done = new ManualResetEvent(false); | |||
pakrym
force-pushed
the
pakrym/SIGTERM
branch
from
4390e0e to
2401365
Compare
pakrym
changed the title
| #endif | ||
| Console.CancelKeyPress += (sender, eventArgs) => | ||
| { | ||
| shutdown(); |
There was a problem hiding this comment.
The behavior of shutdown() is now synchronous vs. asynchronous. So long as CancelKeyPress comes in on a random background thread, this shouldn't matter. Just pointing out there may be a behavioral difference from the existing code.
No description provided.