Fix FastKillOnCancelKeyPress not stopping the process.

[jsukhabut]

When FastKillOnCancelKeyPress=true State is set to Closing but Stop process never starts.

[jsukhabut]

Issue #3933

[ReubenBond]

This and the above IConfigurationValidator change seem orthogonal to this PR - maybe they should be moved into a separate PR

[jsukhabut]

Sorry I am new to this. I thought checking this into my fork would not affect this pull request. This issue is something else. When ClusterId is not set you get a Azure Storage Error "Bad Request". I was had to figure out what was wrong. This check-in was meant for my branch.

[ReubenBond]

No need to apologize :)
You can revert those changes in this branch and they'll disappear from here.

If you'd like assistance, let me know.

[ReubenBond]

Why this OnStop call here when OnStop will be called via StopAsync below?

[jsukhabut]

Because the OnStop after call will get into the mode that just waits for State to Terminate, but nothing signals the lifecycle to stop.

[ReubenBond]

It seems to me that the correct fix here is to not set SystemStatus directly at all and just call Shutdown() (or Stop(), but my leaning is towards graceful shutdown on Ctrl+C)

EDIT: to clarify, what's happening today is that when someone hits Ctrl+C, the handler sets the SystemStatus to Stopping and then calls Stop() which sees that the SystemStatus is already not Running (and therefore the system is already stopping) and loops forever, waiting for a termination which will never come.

[galvesribeiro]

I'm having the same issue you described @ReubenBond

[jsukhabut]

The way I worked around the issue in my console app is to turn FastKillOnCancelKeyPress=false and handle the cancel myself.

Console.CancelKeyPress += Console_CancelKeyPress;

    private void Console_CancelKeyPress(object sender, ConsoleCancelEventArgs e)
    {
        Console.WriteLine("*****************Shutdown Started********************");
        cancelToken.Cancel();
        silo.StopAsync(cancelToken.Token);            
    }

Update:

Still not working for me in Kubernetes with this override. Probably due to this line in Orleans.
AppDomain.CurrentDomain.ProcessExit += HandleProcessExit;

[ReubenBond]

@Jms69 that's how I think it should be done. We should remove the option and allow end-users to configure it themselves.

Particularly since Microsoft.Extension.Hosting will have an abstraction for controlling lifetime of a collection of hosted services.

[ReubenBond]

@Jms69 I submitted a PR against your fork: jsukhabut#1

If it works for you then you can merge it and this PR will automatically update

[jsukhabut]

I merged the pull request and do some more testing. I am trying to get scale down to work in AKS Kubernetes cluster.

As you mentioned earlier it would be cool if we can provide our own cancel action in case we wanted to do some extra shutdown code:

builder.ConfigurationCancelAction(Action< ISiloHost > action);

[jsukhabut]

Scale up/down in Kubernetes now works perfectly.

[ReubenBond]

@Jms69

it would be cool if we can provide our own cancel action in case we wanted to do some extra shutdown code:
builder.ConfigurationCancelAction(Action action);

That's the idea behind the Generic Host Builder which will be released alongside .NET Core 2.1. Once that's available we will work to strip away this functionality and rely on that.

[ReubenBond]

I'm hoping we can have one more set of eyes on this. I'm happy with it. Please merge if you're happy with it.

[sergeybykov]

Thank you, @Jms69!

[benjaminpetit]

From what I understand, here you are doing a clean shutdown, not a fast kill like before (and as the name in the config suggest). I think by default, we should not attempt to do a clean shutdown.

Or at the very least, rename the current config flag

[ReubenBond]

Good point, the config flag name implies abrupt termination - which I would imagine looks like Environment.FaskKill(). Either way, I'm happy for it to be changed to this.Stop() instead this.Shutdown().

Longer term, Ctrl+C should at least attempt a clean shutdown the first time it's pressed, but this is a concern which we should shift to the generic host.