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

Make the way configurable how hosting is kept alive #129

Closed
wants to merge 2 commits into from

Conversation

Tragetaschen
Copy link
Contributor

I'm currently trying to host on Linux and integrated into systemd as init system. The fixed default of Console.ReadLine() to keep the hosting running does not work well and I would like to change this in my application so I can react properly to SIGTERM on Linux. This PR adds an interface and the current default implementation to Hosting.

Within a web application, this default can be overridden by

services.AddInstance<IHostingKeepAlive>(new MyHostingKeepAlive());

@davidfowl
Copy link
Member

What does MyHostingKeepAlive look like? Why wouldn't we bake this into hosting in general?

@Tragetaschen
Copy link
Contributor Author

Some background on service killing with systemd: http://www.freedesktop.org/software/systemd/man/systemd.kill.html

Sample Hold()-implementation with Mono.Posix:

public void Hold()
{
    var unixSignal = new UnixSignal(Mono.Unix.Native.Signum.SIGTERM);
    unixSignal.WaitOne();
}

@Tragetaschen
Copy link
Contributor Author

With regards to a notification on successful startup, another scenario is notifying systemd that the process finished starting up. The cleanest solution is to acquire a D-Bus name once the process is done booting up.

@mstaessen
Copy link

This is the exact issue that I wanted to address with aspnet/dnx#494.

Seems to me that hosting indeed needs to be configurable as this is probably something that will differ for each hosting platform...

@victorhurdugaci
Copy link
Contributor

@Tragetaschen can you please get this rebased on top of the dev branch?

@Tragetaschen
Copy link
Contributor Author

Sure

@victorhurdugaci
Copy link
Contributor

Thanks for rebasing! We have a bot that checks for CLAs on PRs. Since this PR is pretty old, the bot did not analyze it.

Can you please close this PR and recreate it?

@davidfowl
Copy link
Member

He's good to go. He's a regular in these here parts

@victorhurdugaci
Copy link
Contributor

👍 then I'll merge this today

@Tragetaschen
Copy link
Contributor Author

Here is a sample implementation when being hosted on Linux using systemd.

@davidfowl
Copy link
Member

I would rename Hold to WaitForSignal or Wait

@davidfowl
Copy link
Member

Also /cc @lodejard on this to see if he likes the interface

@Tragetaschen
Copy link
Contributor Author

I was thinking about turning this into a Task-returning *Async method and reworking the shutdown-logic in Program.cs to

  • Use more using
  • Stop relying on Task.Run (which will move into the fallback default implementation with Console.ReadLine)
  • Simplify the Registered action to just a TCS' Task

That way, the error handing during shutdown can also be simplified.
I'd guess with the exception of Console.ReadLine, most of the Hold implementations will—like the systemd example—benefit from a Task based API.

Regarding the name of the method, I'm not happy either. The problem is that the interface and the method describe the same thing, one as a noun and the other using a verb. A "logical" choice would be KeepAlive[Async](), but that's weird. With a Task return value, this could be SetupAsync with a documentation that completing the returned Task will initiate the shutdown.

@lodejard
Copy link
Contributor

I really like this change but I don't think it needs to be an entirely new service. Let's see what it would be like if a property was added to IApplicationLifetime of public Action ApplicationStarted {get;set;}

The ApplicationStarted is more of an event than a blocking or async method. The default Action should be the lines:

            ApplicationStarted = () =>
            {
                var ignored = Task.Run(() =>
                {
                    Console.WriteLine("Started");
                    Console.ReadLine();
                    appShutdownService.RequestShutdown();
                });
            }

@davidfowl
Copy link
Member

An assignable action looks weird. Event? Also how do I make sure the application exists if any signal is tripped

@Tragetaschen
Copy link
Contributor Author

I have implemented my sketch and updated the sample.

I do understand the tendency to not make this an interface. It's just for one method and the cost of documenting is non-trivial. But I think the alternatives will look weird when applied in an actual application:

  • Assigning an action indeed looks weird
  • An event is counterintuitive, because programmers learned not to spend too much time in an event handler. Here, the event handler defines the entire lifetime of the application.
  • It's hard to provide an implementation from a library

In the end, how the application keeps running is an orthogonal concept to anything else and I think this justifies an interface on its own.

@victorhurdugaci
Copy link
Contributor

@lodejard , @davidfowl can we please close on this and get a final design/solution? It is blocking the Nano Server runs...

@HaoK
Copy link
Member

HaoK commented Apr 28, 2015

Maybe I'm crazy, or just getting used to Lou, but I think I like the assignable action better too...

Although it would be nice if we had some sugar/overloads to simplify it down to just the inner real Action?:

appLifetime.WhenStarted(() => 
     {
          Console.WriteLine("Started");
          Console.ReadLine();
          appShutdownService.RequestShutdown();
     });

@dnfclas
Copy link

dnfclas commented May 7, 2015

@Tragetaschen, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

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

Successfully merging this pull request may close these issues.

7 participants