-
Notifications
You must be signed in to change notification settings - Fork 402
Add AbandonOnRepeatCancellation extension method #1227
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@jonsequitur Adding unit-tests for this functionality is kinda hard, since unit-tests and Ctrl+C signaling don't really work well together. Instead I created an additional Console application project for manual testing of Sample output from that test application: (shortened for brevity)
Here we can see a Command handler that does not return from the invocation when the cancellation token is cancelled (in this case the After the initial Ctrl+C press (line 3), cancellation of the invocation is requested, but the handler does not return, and so the program continues running. At line 5 Ctrl+C was pressed a second time, causing the invocation to be abandoned and control flow immediately returns back to |
@@ -127,6 +129,51 @@ public static CommandLineBuilder CancelOnProcessTermination(this CommandLineBuil | |||
return builder; | |||
} | |||
|
|||
public static CommandLineBuilder AbandonOnRepeatCancellation( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if enabling this functionality would make sense as a parameter to CancelOnProcessTermination
rather than a separate method.
Thoughts, @tmds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe as an optional parameter with a struct/class containing behaviour options. Among them bool abandon and int threshold?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think we should add it to CancelOnProcessTermination
to avoid weird interactions like https://github.com/dotnet/command-line-api/pull/1227/files#r605602596.
CancelOnProcessTermination
gets included by UseDefaults
. There is no pattern for passing patterns to the default middlewares that are used by UseDefaults
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there tools that you've seen where the force quit behavior would involve more than two CTRL-C presses? I'm wondering if the threshold needs to be configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, okay, we don't need a threshold. But @tmds has a good point. Should UseDefault include this abandoning behaviour? How would someone use UseDefault and abandon on secondary cancel?
Should we maybe also inject a state object into the Invocation that keeps a count of cancellations, so that people could react to a secondary cancel? In hosting the StopAsync method accepts a cancellation token to abort graceful shutdown and just force shutdown the host. We could use such an injected status object to control cancellation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intuition is that this behavior should be enabled by default so that the path of least resistance for the developer leads to the most consistent experience for end users.
{ | ||
builder.AddMiddleware(async (context, next) => | ||
{ | ||
var initialCancelToken = context.GetCancellationToken(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't call GetCancellationToken
here. When the CancellationToken
gets requested (which is assumed to be done by the user), the context assumes that CancellationToken
will be used to implement Ctrl+C
handling, and the app no longer terminates by default.
So this has the unintended side-effect that command handlers which don't use a CancellationToken
no longer terminate on first Ctrl+C
.
TL;DR
AbandonOnRepeatCancellation
toCommandLineBuidler
.Description
When using the
CancelOnProcessTermination
middleware (or similar mechanism) aCancellationToken
is added to theInvocationContext
. TheCancelOnProcessTermination
middleware monitors theConsole.CancelKeyPress
event and cancels the token when a console cancellation key press is intercepted. After that is waits for the command-handler to return.Since the
CancelOnProcessTermination
middleware suppresses the termination of the process to allow for a graceful shutdown and return, this behaviour can lead to the application not being terminable if the command-handler does not monitor the invocation cancellation token or is stuck in a critical section where monitoring the token is not viable.This proposal adds another middleware that will wait for the first cancellation to occur and then registers an additional event handler for the
Console.CancelKeyPress
event. When a repeated cancellation signal is intercepted, this middleware abandons the invocation and immediately returns, disregarding the state of the invocation.Note that the invocation cannot really be forcibly terminated, the name
abandon
really means that the original invocation will continue to run in the background. However, the pipeline will terminate returning directly back to main. Most likely,main
will either just return terminating the application immediately anyways, or elsemain
may contain som critical clean-up code that will now have the chance to run.This will enable the following application pattern: