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

Third way of making configurable how hosting is kept alive #255

Merged
merged 1 commit into from
May 12, 2015

Conversation

Tragetaschen
Copy link
Contributor

I have tried to incorporate the comments on the other two approaches (#129 and #253) to tackle the problem from yet another angle.

This now entirely relies on IApplicationShutdown as the method of communication and instead of going back and forth between Program and the keep-alive implementation, the latter is now responsible for doing whatever is required to call RequestShutdown(). For things like Console.CancelKeyPressed, this is the simplest event handler imaginable.

Additionally, it's now possible to register any number of IHostingKeepAlive implementations and any one can call RequestShutdown() independently.

The default implementation can be IServiceCollection.Replaced, but keep in mind the open issue and discussion from aspnet/DependencyInjection#155.

As a bonus, the implementation in Program.cs is now as simple as it can get.

@lodejard @davidfowl @victorhurdugaci @HaoK

@ghost ghost added the cla-already-signed label May 5, 2015
@@ -90,6 +90,7 @@ private void EnsureApplicationServices()
{
EnsureStartup();

_applicationServiceCollection.AddSingleton<IHostingKeepAlive, ConsoleHostingKeepAlive>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation should be registered in Program.cs. Helios (the IIS layer) also uses this library.

@Tragetaschen Tragetaschen force-pushed the hosting-keep-alive branch 2 times, most recently from 3c33d74 to 4316b4f Compare May 5, 2015 11:25
@Tragetaschen
Copy link
Contributor Author

I have updated my sample code

@victorhurdugaci
Copy link
Contributor

I don't really like the name of the interface IHostingKeepAlive. It implies that it is used to keep the hosting alive. It has nothing to do with that.

If the implementation doesn't call one of the termination methods, then the code will keep running forever. It would be a keep alive contract if it would block or return a cancellation token.

appShutdownService.RequestShutdown();
});
foreach (var hostingKeepAlive in hostingKeepAlives)
hostingKeepAlive.Setup(appShutdownService);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Coding style: always use { }
  2. This approach doesn't allow me to overwrite the default behavior and that's a critical scenario for the Nano server

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well we actually do have this pattern already today with multiservices, its not great but you can override the default behavior by removing the IHostingKeepAliveService and adding your own.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I meant by IServiceCollection.Replace. There is an extension method for that in DependencyInjection.

@Tragetaschen
Copy link
Contributor Author

IShutdownMethod?

@HaoK
Copy link
Member

HaoK commented May 5, 2015

IApplicationShutdownHandler / Handle (instead of Setup)?

@victorhurdugaci
Copy link
Contributor

Isn't it weird that the "shutdown" handler writes "started"? 😄

@Tragetaschen
Copy link
Contributor Author

Hehe. See my samples for some better texts. It should be something along the lines of To terminate, press enter or similar.

@HaoK
Copy link
Member

HaoK commented May 5, 2015

IShutdownManager might not be bad either, since its his job to request shutdown...

@Tragetaschen
Copy link
Contributor Author

Well, then all the way: IShutdownRequester. "Manager" is such an overloaded term…

@HaoK
Copy link
Member

HaoK commented May 5, 2015

Ok, then my vote is back to IShutdownHandler / Handle(IApplicationShutdown)

@Eilon Eilon closed this May 5, 2015
@Eilon Eilon reopened this May 5, 2015
@Tragetaschen
Copy link
Contributor Author

Huh, cla-required?

@Eilon
Copy link
Member

Eilon commented May 5, 2015

@Tragetaschen We just changed the repos from MS Open Tech to .NET Foundation so there's a different CLA. The new CLA is simpler than the old one and so should only take a few moments to re-sign. Apologies for the extra bit of work!

@Tragetaschen Tragetaschen force-pushed the hosting-keep-alive branch 2 times, most recently from deafb77 to bc87bbf Compare May 7, 2015 08:10
@Tragetaschen
Copy link
Contributor Author

I have submitted the .NET Foundation's CLA, but @dnfclas doesn't recognize it yet. Maybe I did something wrong? @danroth27

@Tragetaschen Tragetaschen force-pushed the hosting-keep-alive branch from bc87bbf to d6d2c0f Compare May 7, 2015 08:25
@Tragetaschen
Copy link
Contributor Author

Back to topic: Yes, indeed, IShutdownHandler concisely covers the the intention of the interface.

However, I'm having a harder time with the method name. I tried to write the API documentation when the name was Handle, but couldn't come up with a sentence that made it the logical conclusion. Setup on the other hand strongly points to the do-something-and-return mentality of the method.

@HaoK
Copy link
Member

HaoK commented May 7, 2015

Perhaps something like: "This method is responsible for handling shutdown and eventually calling RequestShutdown."

@davidfowl
Copy link
Member

After thinking through this a bit I'd (and after a discussion with @victorhurdugaci), I'm wondering why we need any of this at all? We can just always wait on the reset event and nothing else. If you want to setup something then just do it in your Configure method. Hook whatever it is you need and then call RequestShutdown at the appropriate time.

@Tragetaschen
Copy link
Contributor Author

And an app.UseDefaultShutdown() to get the the Console.ReadLine() behavior?

@victorhurdugaci
Copy link
Contributor

We want a default behavior that allows the server to shutdown gracefully, but that behavior has to be override-able.

@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;

@davidfowl
Copy link
Member

I actually think is the best approach. What happens when you do ctrl + c right now?

@Tragetaschen
Copy link
Contributor Author

This ends up calling the equivalent of Environment.Exit on both Mono and Windows and runs the Finalizers, not the Dispose logic. Btw, it's that finalizer code path that currently blocks Kestrel when doing CTRL+C.

@davidfowl
Copy link
Member

@Tragetaschen so as it is right now, things will hang for kestrel without the graceful unwind? We could just add the control c handling then.

@Tragetaschen
Copy link
Contributor Author

The main problem with Console.ReadLine is, that it requires a working TTY on Unix. That's the reason why this specific code needs to be removed from Hosting, so a Docker image or a systemd service can remove this superfluous requirement.

On the other hand, I don't think it's necessary to overriding/replace a default CTRL+C handler when Hosting registers it. You would only need this, if you don't want the process to shutdown and that's quite counter-intuitive when actually running on a terminal.
All other requirements can be easily wired against the members of IApplicationLifetime.

(dang, you beat me with your comment)

@davidfowl
Copy link
Member

@Tragetaschen add that to your change

@Tragetaschen Tragetaschen force-pushed the hosting-keep-alive branch 2 times, most recently from 64f16dc to 2e8c301 Compare May 8, 2015 10:21
@Tragetaschen
Copy link
Contributor Author

The Console.CancelKeyPress hasn't landed in .NET Core (System.Console 4.0.0-beta-22906) yet. I've if'ed that part and left a comment

@davidfowl
Copy link
Member

@ellismg @ericstj Why don't we have that change yet in our builds? Has it made it in?

@Tragetaschen Tragetaschen force-pushed the hosting-keep-alive branch from 2e8c301 to 19fc9fb Compare May 8, 2015 10:46
@ericstj
Copy link

ericstj commented May 8, 2015

Looks like @ellismg didn't update the reference assembly.

@ericstj
Copy link

ericstj commented May 8, 2015

I made a fix to the reference assembly, you should see it in the next build.

@Tragetaschen Tragetaschen force-pushed the hosting-keep-alive branch 2 times, most recently from cee0a48 to fcf4573 Compare May 11, 2015 21:37
@Tragetaschen
Copy link
Contributor Author

4.0.0-beta-22910 has landed and includes CancelKeyPress

👏 😄

Don't require a TTY on Unix.
davidfowl added a commit that referenced this pull request May 12, 2015
Third way of making configurable how hosting is kept alive
@davidfowl davidfowl merged commit e19289f into aspnet:dev May 12, 2015
@Tragetaschen Tragetaschen deleted the hosting-keep-alive branch May 12, 2015 06:53
@victorhurdugaci
Copy link
Contributor

How do you overwrite the Ctrl+C behavior?

@Tragetaschen
Copy link
Contributor Author

Why would you want to do that?

As discussed above, when not running in a console environment, the registered handler is never called and you can still implement any method you want to call IApplicationShutdown.RequestShutdown().

If you are running in a console environment, that's an interactive environment and properly reacting to CTRL+C like here is a genuinely good idea.

Still, I might be missing something, but right now I think a good answer to your question is: You don't need to.

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