-
Notifications
You must be signed in to change notification settings - Fork 401
Add support for timeout in process termination handling #1756
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
Conversation
ac56679
to
680d466
Compare
873530b
to
a98a0ba
Compare
...ility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt
Outdated
Show resolved
Hide resolved
src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs
Outdated
Show resolved
Hide resolved
@jonsequitur thank you for the initial feedback (should be addressed now). Are there any further unaddressed concerns or suggestions? |
src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs
Outdated
Show resolved
Hide resolved
src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs
Outdated
Show resolved
Hide resolved
I suggested a couple of test renames but feel free to adjust if you have something more precise. The goal is to embed the expectations about the scenario in the test name, like a spec. |
[LinuxOnlyTheory] | ||
[InlineData(SIGINT)] | ||
[InlineData(SIGTERM)] | ||
public async Task CancelOnProcessTermination_null_timeout_on_cancel_processing(int signo) |
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.
Is this test redundant with the pre-existing test above?
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.
It is redundand with the current CancelOnProcessTermination
implementation. But it guards the possibility of contract/behavior change on explict passing of null
as timeout
.
I can remove it if you feel it's superfluous.
@jonsequitur Are there any further unaddressed concerns or suggestions? |
Background:
related to dotnet/templating#4799
It would be helpfull to be able to specify optional maximum timeout for handling the process termination - in order to prevent unwanted 'unkillable' command - especially for cases where command builder and and actual implementation of the command have different maintainers (e.g. a case of dotnet sdk).
The process that don't repond to Ctrl-C can still be killed by other means - but it may create unpleasant and inconsistent experience to users. The idea is to have top level handler that gives command chance to perform cleanup in correct way, but bail out if they do not return in timely manner (e.g. due to not using the cancellation patter at all).
If the optinal timeout is not specified, the original behavior remains (no timeout enforced).