Skip to content
This repository has been archived by the owner on Dec 19, 2018. It is now read-only.

Handle SIGTERMs for graceful shutdown #870

Closed
halter73 opened this issue Oct 28, 2016 · 17 comments
Closed

Handle SIGTERMs for graceful shutdown #870

halter73 opened this issue Oct 28, 2016 · 17 comments
Assignees
Milestone

Comments

@halter73
Copy link
Member

halter73 commented Oct 28, 2016

@stepro pointed out it should now be possible to handle SIGTERMs in addition to the SIGINTs (CTRL+C) that we already hook for graceful shutdown.

The managed API for SIGTERM is exposed by the AssemblyLoadContext.Unloading event, which is raised when the AppDomain.ProcessExit event is raised. I know this only because I’ve seen it used in the VSTS agent codebase (see this, line 44).
You can see how coreclr maps SIGTERM to shutting down here, line 170.

This should be especially useful for running in docker containers.

@pakrym pakrym self-assigned this Oct 28, 2016
@davidfowl
Copy link
Member

Do we want to handle both SIGTERM and SIGINT? Should they mean the same thing? Here are related issues:

@muratg muratg added this to the 1.2.0 milestone Oct 31, 2016
@muratg
Copy link

muratg commented Oct 31, 2016

cc @glennc

@stepro
Copy link

stepro commented Oct 31, 2016

As it stands, .NET Core handles both SIGTERM and SIGINT (as well as most of the other Linux signals) in the sense that it registers for the signals. Whether it actually does something useful is up to whether the application listens on the necessary events and handles them appropriately.

Given this, ASP .NET should definitely gracefully stop the application on SIGTERM because the alternative (effectively, SIGTERM equals SIGKILL) is never going to be the desired behavior.

Aside from potential differences on what is output to a live console, SIGINT and SIGTERM should both stop the ASP .NET application. SIGINT works well for a dev scenario where you are locally running the process synchronously; SIGTERM is appropriate when running in the background or unattended in a server scenario.

@davidfowl
Copy link
Member

davidfowl commented Oct 31, 2016

I can't get this to work in a simple console app on OSX:

public class Program
{
    public static void Main(string[] args)
    {
        var ended = new ManualResetEventSlim();
        var starting = new ManualResetEventSlim();

        AssemblyLoadContext.Default.Unloading += ctx =>
        {
            System.Console.WriteLine("Unloding fired");
            starting.Set();
            System.Console.WriteLine("Waiting for completion");
            ended.Wait();
        };

        System.Console.WriteLine("Waiting for signals");
        starting.Wait();

        System.Console.WriteLine("Received signal gracefully shutting down");
        Thread.Sleep(5000);
        ended.Set();
    }
}

I tried using:

kill -SIGTERM {PID}

It just ends without firing any events.

@muratg
Copy link

muratg commented Oct 31, 2016

@davidfowl Does a console app catch SIGINT, though?

@davidfowl
Copy link
Member

Handling Console.CancelKeyPress handles SIGINT but not this.

@muratg muratg modified the milestones: 1.1.0, 1.2.0 Oct 31, 2016
@muratg
Copy link

muratg commented Oct 31, 2016

Bringing the bug back to 1.1 for further investigation.

@stepro
Copy link

stepro commented Oct 31, 2016

The code I got to work pulls the AssemblyLoadContext in a different way:

AssemblyLoadContext.GetLoadContext(typeof(Program).GetTypeInfo().Assembly)

@halter73
Copy link
Member Author

I just tried your program verbatim on Debian 8.2. Worked fine there. Maybe the issue you're running into is mac specific.

 ~/SigtermApp$ dotnet run
Project SigtermApp (.NETCoreApp,Version=v1.1) was previously compiled. Skipping compilation.
Waiting for signals
Unloding fired
Waiting for completion
Received signal gracefully shutting down
~/SigtermApp$

@halter73
Copy link
Member Author

BTW kill <PID> and kill -SIGTERM <PID> are the same.

@davidfowl
Copy link
Member

Then it's an OSX specific problem. We should look into this.

/cc @kouvel any ideas?

@pakrym
Copy link
Contributor

pakrym commented Nov 2, 2016

#876

@muratg
Copy link

muratg commented Nov 4, 2016

@pakrym test in yet? This one good to close?

@pakrym
Copy link
Contributor

pakrym commented Nov 4, 2016

@muratg tests is still pending

@Eilon
Copy link
Member

Eilon commented Nov 9, 2016

@pakrym please label appropriately and close ASAP.

@pakrym
Copy link
Contributor

pakrym commented Nov 9, 2016

#878

@pakrym pakrym closed this as completed Nov 9, 2016
@Eilon
Copy link
Member

Eilon commented Nov 9, 2016

@pakrym @muratg should this issue be marked as a bug?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants