From a98a0ba030a1c4bd1b20a1771e2e05fb2fa09137 Mon Sep 17 00:00:00 2001 From: Jan Krivanek Date: Mon, 6 Jun 2022 21:01:28 +0200 Subject: [PATCH 1/6] Add support for timeout in process termination handling --- ...ommandLine_api_is_not_changed.approved.txt | 2 +- .../CancelOnProcessTerminationTests.cs | 79 ++++++++++++++++++- .../Builder/CommandLineBuilderExtensions.cs | 51 +++++++++--- 3 files changed, 120 insertions(+), 12 deletions(-) diff --git a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt index 35bddbdbe2..ddc12ac786 100644 --- a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt +++ b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt @@ -240,7 +240,7 @@ System.CommandLine.Builder public static class CommandLineBuilderExtensions public static CommandLineBuilder AddMiddleware(this CommandLineBuilder builder, System.CommandLine.Invocation.InvocationMiddleware middleware, System.CommandLine.Invocation.MiddlewareOrder order = Default) public static CommandLineBuilder AddMiddleware(this CommandLineBuilder builder, System.Action onInvoke, System.CommandLine.Invocation.MiddlewareOrder order = Default) - public static CommandLineBuilder CancelOnProcessTermination(this CommandLineBuilder builder) + public static CommandLineBuilder CancelOnProcessTermination(this CommandLineBuilder builder, System.Nullable cancelationProcessingTimeout = null) public static CommandLineBuilder EnableDirectives(this CommandLineBuilder builder, System.Boolean value = True) public static CommandLineBuilder EnableLegacyDoubleDashBehavior(this CommandLineBuilder builder, System.Boolean value = True) public static CommandLineBuilder EnablePosixBundling(this CommandLineBuilder builder, System.Boolean value = True) diff --git a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs index ef54677dde..ec5972c8d0 100644 --- a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs +++ b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs @@ -20,7 +20,7 @@ public class CancelOnProcessTerminationTests private const int SIGTERM = 15; [LinuxOnlyTheory] - [InlineData(SIGINT, Skip = "https://github.com/dotnet/command-line-api/issues/1206")] // Console.CancelKeyPress + [InlineData(SIGINT/*, Skip = "https://github.com/dotnet/command-line-api/issues/1206"*/)] // Console.CancelKeyPress [InlineData(SIGTERM)] // AppDomain.CurrentDomain.ProcessExit public async Task CancelOnProcessTermination_cancels_on_process_termination(int signo) { @@ -91,6 +91,83 @@ public async Task CancelOnProcessTermination_cancels_on_process_termination(int process.ExitCode.Should().Be(CancelledExitCode); } + [LinuxOnlyTheory] + [InlineData(null, SIGINT)] + [InlineData(100, SIGINT, Skip = "Doesn't work under RemoteExecutor - process not terminated after Environment.Exit")] + [InlineData(null, SIGTERM)] + [InlineData(100, SIGTERM, Skip = "Doesn't work under RemoteExecutor - process not terminated after ExitProcess event")] + public async Task CancelOnProcessTermination_timeout_on_cancel_processing(int? timeOutMs, int signo) + { + TimeSpan? timeOut = timeOutMs.HasValue ? TimeSpan.FromMilliseconds(timeOutMs.Value) : null; + + const string ChildProcessWaiting = "Waiting for the command to be cancelled"; + const int CancelledExitCode = 42; + const int ForceTerminationCode = 130; + + Func> childProgram = (string[] args) => + { + var command = new Command("the-command"); + + command.SetHandler(async context => + { + var cancellationToken = context.GetCancellationToken(); + + try + { + context.Console.WriteLine(ChildProcessWaiting); + await Task.Delay(int.MaxValue, cancellationToken); + context.ExitCode = 1; + } + catch (OperationCanceledException) + { + // For Process.Exit handling the event must remain blocked as long as the + // command is executed. + // We are currently blocking that event because CancellationTokenSource.Cancel + // is called from the event handler. + // We'll do an async Task.Delay now. This means the Cancel call will return + // and we're no longer actively blocking the event. + // The event handler is responsible to continue blocking until the command + // has finished executing. If it doesn't we won't get the CancelledExitCode. + await Task.Delay(TimeSpan.FromMilliseconds(2000)); + + context.ExitCode = CancelledExitCode; + } + + }); + + return new CommandLineBuilder(new RootCommand + { + command + }) + .CancelOnProcessTermination(/*timeOut*/TimeSpan.FromSeconds(100)) + .Build() + .InvokeAsync("the-command"); + }; + + using RemoteExecution program = RemoteExecutor.Execute(childProgram, psi: new ProcessStartInfo { RedirectStandardOutput = true }); + + Process process = program.Process; + + // Wait for the child to be in the command handler. + string childState = await process.StandardOutput.ReadLineAsync(); + childState.Should().Be(ChildProcessWaiting); + + // Request termination + kill(process.Id, signo).Should().Be(0); + + // Verify the process terminates timely + bool processExited = process.WaitForExit(10000); + if (!processExited) + { + process.Kill(); + process.WaitForExit(); + } + processExited.Should().Be(true); + + // Verify the process exit code + process.ExitCode.Should().Be(timeOutMs.HasValue ? ForceTerminationCode : CancelledExitCode); + } + [DllImport("libc", SetLastError = true)] private static extern int kill(int pid, int sig); } diff --git a/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs b/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs index d226ab9185..3dfcd2bbd7 100644 --- a/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs +++ b/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs @@ -10,6 +10,7 @@ using System.Linq; using System.Reflection; using System.Threading; +using System.Threading.Tasks; using static System.Environment; using Process = System.CommandLine.Invocation.Process; @@ -42,9 +43,21 @@ public static class CommandLineBuilderExtensions /// Enables signaling and handling of process termination via a that can be passed to a during invocation. /// /// A command line builder. + /// + /// Optional timeout for the command to process the exit cancellation. + /// If not passed, or passed null or non-positive timeout (including ), no timeout is enforced. + /// If positive value is passed - command is forcefully terminated after the timeout with exit code 130 (as if was not called). + /// Host enforced timeout for ProcessExit event cannot be extended - default is 2 seconds: https://docs.microsoft.com/en-us/dotnet/api/system.appdomain.processexit?view=net-6.0. + /// /// The same instance of . - public static CommandLineBuilder CancelOnProcessTermination(this CommandLineBuilder builder) + public static CommandLineBuilder CancelOnProcessTermination( + this CommandLineBuilder builder, + TimeSpan? cancelationProcessingTimeout = null) { + cancelationProcessingTimeout ??= Timeout.InfiniteTimeSpan; + // https://tldp.org/LDP/abs/html/exitcodes.html - 130 - script terminated by ctrl-c + const int SIGINT_EXIT_CODE = 130; + builder.AddMiddleware(async (context, next) => { bool cancellationHandlingAdded = false; @@ -56,14 +69,8 @@ public static CommandLineBuilder CancelOnProcessTermination(this CommandLineBuil { blockProcessExit = new ManualResetEventSlim(initialState: false); cancellationHandlingAdded = true; - consoleHandler = (_, args) => - { - cts.Cancel(); - // Stop the process from terminating. - // Since the context was cancelled, the invocation should - // finish and Main will return. - args.Cancel = true; - }; + // Default limit for ProcesExit handler is 2 seconds + // https://docs.microsoft.com/en-us/dotnet/api/system.appdomain.processexit?view=net-6.0 processExitHandler = (_, _) => { cts.Cancel(); @@ -71,9 +78,33 @@ public static CommandLineBuilder CancelOnProcessTermination(this CommandLineBuil // We provide a return value using Environment.ExitCode // because Main will not finish executing. // Wait for the invocation to finish. - blockProcessExit.Wait(); + if (!blockProcessExit.Wait(cancelationProcessingTimeout > TimeSpan.Zero + ? cancelationProcessingTimeout.Value + : Timeout.InfiniteTimeSpan)) + { + context.ExitCode = SIGINT_EXIT_CODE; + } ExitCode = context.ExitCode; }; + consoleHandler = (_, args) => + { + cts.Cancel(); + // Stop the process from terminating. + // Since the context was cancelled, the invocation should + // finish and Main will return. + args.Cancel = true; + if (cancelationProcessingTimeout! > TimeSpan.Zero) + { + Task + .Delay(cancelationProcessingTimeout.Value, default) + .ContinueWith(t => + { + // Prevent our ProcessExit from intervene and block the exit + AppDomain.CurrentDomain.ProcessExit -= processExitHandler; + Environment.Exit(SIGINT_EXIT_CODE); + }, (CancellationToken)default); + } + }; Console.CancelKeyPress += consoleHandler; AppDomain.CurrentDomain.ProcessExit += processExitHandler; }; From 3ed2081d08a1fea11004a77f4685320eada8197e Mon Sep 17 00:00:00 2001 From: Jan Krivanek Date: Tue, 7 Jun 2022 22:05:51 +0200 Subject: [PATCH 2/6] Fix tests - do not rely on captured closure --- .../CancelOnProcessTerminationTests.cs | 98 +++++++++++++++++-- 1 file changed, 92 insertions(+), 6 deletions(-) diff --git a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs index ec5972c8d0..c2ad5ceb66 100644 --- a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs +++ b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs @@ -93,10 +93,8 @@ public async Task CancelOnProcessTermination_cancels_on_process_termination(int [LinuxOnlyTheory] [InlineData(null, SIGINT)] - [InlineData(100, SIGINT, Skip = "Doesn't work under RemoteExecutor - process not terminated after Environment.Exit")] [InlineData(null, SIGTERM)] - [InlineData(100, SIGTERM, Skip = "Doesn't work under RemoteExecutor - process not terminated after ExitProcess event")] - public async Task CancelOnProcessTermination_timeout_on_cancel_processing(int? timeOutMs, int signo) + public async Task CancelOnProcessTermination_null_timeout_on_cancel_processing(int? timeOutMs, int signo) { TimeSpan? timeOut = timeOutMs.HasValue ? TimeSpan.FromMilliseconds(timeOutMs.Value) : null; @@ -124,22 +122,110 @@ public async Task CancelOnProcessTermination_timeout_on_cancel_processing(int? t // command is executed. // We are currently blocking that event because CancellationTokenSource.Cancel // is called from the event handler. - // We'll do an async Task.Delay now. This means the Cancel call will return + // We'll do an async Yield now. This means the Cancel call will return // and we're no longer actively blocking the event. // The event handler is responsible to continue blocking until the command // has finished executing. If it doesn't we won't get the CancelledExitCode. - await Task.Delay(TimeSpan.FromMilliseconds(2000)); + await Task.Yield(); + + // Exit code gets set here - but then execution continues and is let run till code voluntarily returns + // hence exit code gets overwritten below + context.ExitCode = 123; + } + + // This is an example of bad pattern and reason why we need a timeout on termination processing + await Task.Delay(TimeSpan.FromMilliseconds(200)); + + context.ExitCode = CancelledExitCode; + }); + + return new CommandLineBuilder(new RootCommand + { + command + }) + // Unfortunately we cannot use test parameter here - RemoteExecutor currently doesn't capture the closure + .CancelOnProcessTermination(null) + .Build() + .InvokeAsync("the-command"); + }; + + using RemoteExecution program = RemoteExecutor.Execute(childProgram, psi: new ProcessStartInfo { RedirectStandardOutput = true }); + + Process process = program.Process; + + // Wait for the child to be in the command handler. + string childState = await process.StandardOutput.ReadLineAsync(); + childState.Should().Be(ChildProcessWaiting); + + // Request termination + kill(process.Id, signo).Should().Be(0); + + // Verify the process terminates timely + bool processExited = process.WaitForExit(10000); + if (!processExited) + { + process.Kill(); + process.WaitForExit(); + } + processExited.Should().Be(true); + + // Verify the process exit code + process.ExitCode.Should().Be(timeOutMs.HasValue ? ForceTerminationCode : CancelledExitCode); + } + + [LinuxOnlyTheory] + [InlineData(100, SIGINT)] + [InlineData(100, SIGTERM)] + public async Task CancelOnProcessTermination_timeout_on_cancel_processing(int? timeOutMs, int signo) + { + TimeSpan? timeOut = timeOutMs.HasValue ? TimeSpan.FromMilliseconds(timeOutMs.Value) : null; + + const string ChildProcessWaiting = "Waiting for the command to be cancelled"; + const int CancelledExitCode = 42; + const int ForceTerminationCode = 130; + + Func> childProgram = (string[] args) => + { + var command = new Command("the-command"); + + command.SetHandler(async context => + { + var cancellationToken = context.GetCancellationToken(); + + try + { + context.Console.WriteLine(ChildProcessWaiting); + await Task.Delay(int.MaxValue, cancellationToken); + context.ExitCode = 1; + } + catch (OperationCanceledException) + { + // For Process.Exit handling the event must remain blocked as long as the + // command is executed. + // We are currently blocking that event because CancellationTokenSource.Cancel + // is called from the event handler. + // We'll do an async Yield now. This means the Cancel call will return + // and we're no longer actively blocking the event. + // The event handler is responsible to continue blocking until the command + // has finished executing. If it doesn't we won't get the CancelledExitCode. + await Task.Yield(); context.ExitCode = CancelledExitCode; } + // This is an example of bad pattern and reason why we need a timeout on termination processing + await Task.Delay(TimeSpan.FromMilliseconds(2000)); + + // Execution should newer get here as termination processing has a timeout of 100ms + context.ExitCode = 123; }); return new CommandLineBuilder(new RootCommand { command }) - .CancelOnProcessTermination(/*timeOut*/TimeSpan.FromSeconds(100)) + // Unfortunately we cannot use test parameter here - RemoteExecutor currently doesn't capture the closure + .CancelOnProcessTermination(TimeSpan.FromMilliseconds(100)) .Build() .InvokeAsync("the-command"); }; From 44d23f6b9b5f3e844fca4658b947f95e1e461513 Mon Sep 17 00:00:00 2001 From: Jan Krivanek Date: Wed, 8 Jun 2022 10:51:46 +0200 Subject: [PATCH 3/6] Improve termination handling to prevent (and test) sync blocking in Cancellation handler --- .../CancelOnProcessTerminationTests.cs | 10 +++++-- .../Builder/CommandLineBuilderExtensions.cs | 26 ++++++++++++++++--- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs index c2ad5ceb66..a2162b5127 100644 --- a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs +++ b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs @@ -211,13 +211,19 @@ public async Task CancelOnProcessTermination_timeout_on_cancel_processing(int? t await Task.Yield(); context.ExitCode = CancelledExitCode; + + // This is an example of bad pattern and reason why we need a timeout on termination processing + await Task.Delay(TimeSpan.FromMilliseconds(1000)); + + // Execution should newer get here as termination processing has a timeout of 100ms + Environment.Exit(123); } // This is an example of bad pattern and reason why we need a timeout on termination processing - await Task.Delay(TimeSpan.FromMilliseconds(2000)); + await Task.Delay(TimeSpan.FromMilliseconds(1000)); // Execution should newer get here as termination processing has a timeout of 100ms - context.ExitCode = 123; + Environment.Exit(123); }); return new CommandLineBuilder(new RootCommand diff --git a/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs b/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs index 3dfcd2bbd7..84862d475b 100644 --- a/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs +++ b/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs @@ -54,10 +54,14 @@ public static CommandLineBuilder CancelOnProcessTermination( this CommandLineBuilder builder, TimeSpan? cancelationProcessingTimeout = null) { - cancelationProcessingTimeout ??= Timeout.InfiniteTimeSpan; // https://tldp.org/LDP/abs/html/exitcodes.html - 130 - script terminated by ctrl-c const int SIGINT_EXIT_CODE = 130; + if (cancelationProcessingTimeout == null || cancelationProcessingTimeout.Value < TimeSpan.Zero) + { + cancelationProcessingTimeout = Timeout.InfiniteTimeSpan; + } + builder.AddMiddleware(async (context, next) => { bool cancellationHandlingAdded = false; @@ -73,7 +77,10 @@ public static CommandLineBuilder CancelOnProcessTermination( // https://docs.microsoft.com/en-us/dotnet/api/system.appdomain.processexit?view=net-6.0 processExitHandler = (_, _) => { - cts.Cancel(); + // Cancel asynchronously not to block the handler (as then the process might possibly run longer then what was the requested timeout) + Task timeoutTask = Task.Delay(cancelationProcessingTimeout.Value); + Task cancelTask = Task.Factory.StartNew(cts.Cancel); + // The process exits as soon as the event handler returns. // We provide a return value using Environment.ExitCode // because Main will not finish executing. @@ -84,15 +91,23 @@ public static CommandLineBuilder CancelOnProcessTermination( { context.ExitCode = SIGINT_EXIT_CODE; } + // Let's block here (to prevent process bailing out) for the rest of the timeout (if any), for cancellation to finish (if it hasn't yet) + else if (Task.WaitAny(timeoutTask, cancelTask) == 0) + { + // The async cancellation didn't finish in timely manner + context.ExitCode = SIGINT_EXIT_CODE; + } ExitCode = context.ExitCode; }; consoleHandler = (_, args) => { - cts.Cancel(); // Stop the process from terminating. // Since the context was cancelled, the invocation should // finish and Main will return. args.Cancel = true; + + // If timeout was requested - make sure cancellation processing (or any other activity within the current process) + // doesn't keep the process running after the timeout if (cancelationProcessingTimeout! > TimeSpan.Zero) { Task @@ -104,6 +119,11 @@ public static CommandLineBuilder CancelOnProcessTermination( Environment.Exit(SIGINT_EXIT_CODE); }, (CancellationToken)default); } + + // Cancel synchronously here - no need to perform it asynchronously as the timeout is already running (and would kill the process if needed), + // plus we cannot wait only on the cancellation (e.g. via `Task.Factory.StartNew(cts.Cancel).Wait(cancelationProcessingTimeout.Value)`) + // as we need to abort any other possible execution within the process - even outside the context of cancellation processing + cts.Cancel(); }; Console.CancelKeyPress += consoleHandler; AppDomain.CurrentDomain.ProcessExit += processExitHandler; From 3c68643d5bd9adcc89d36e511c39cbf1be0eee34 Mon Sep 17 00:00:00 2001 From: Jan Krivanek Date: Wed, 8 Jun 2022 10:57:07 +0200 Subject: [PATCH 4/6] Apply PR comments --- ...ommandLine_api_is_not_changed.approved.txt | 2 +- .../CancelOnProcessTerminationTests.cs | 20 ++++++++----------- .../Builder/CommandLineBuilderExtensions.cs | 18 ++++++++--------- 3 files changed, 18 insertions(+), 22 deletions(-) diff --git a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt index ddc12ac786..77055af8ec 100644 --- a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt +++ b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt @@ -240,7 +240,7 @@ System.CommandLine.Builder public static class CommandLineBuilderExtensions public static CommandLineBuilder AddMiddleware(this CommandLineBuilder builder, System.CommandLine.Invocation.InvocationMiddleware middleware, System.CommandLine.Invocation.MiddlewareOrder order = Default) public static CommandLineBuilder AddMiddleware(this CommandLineBuilder builder, System.Action onInvoke, System.CommandLine.Invocation.MiddlewareOrder order = Default) - public static CommandLineBuilder CancelOnProcessTermination(this CommandLineBuilder builder, System.Nullable cancelationProcessingTimeout = null) + public static CommandLineBuilder CancelOnProcessTermination(this CommandLineBuilder builder, System.Nullable timeout = null) public static CommandLineBuilder EnableDirectives(this CommandLineBuilder builder, System.Boolean value = True) public static CommandLineBuilder EnableLegacyDoubleDashBehavior(this CommandLineBuilder builder, System.Boolean value = True) public static CommandLineBuilder EnablePosixBundling(this CommandLineBuilder builder, System.Boolean value = True) diff --git a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs index a2162b5127..0ca15f5d5c 100644 --- a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs +++ b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs @@ -92,12 +92,10 @@ public async Task CancelOnProcessTermination_cancels_on_process_termination(int } [LinuxOnlyTheory] - [InlineData(null, SIGINT)] - [InlineData(null, SIGTERM)] - public async Task CancelOnProcessTermination_null_timeout_on_cancel_processing(int? timeOutMs, int signo) + [InlineData(SIGINT)] + [InlineData(SIGTERM)] + public async Task CancelOnProcessTermination_null_timeout_on_cancel_processing(int signo) { - TimeSpan? timeOut = timeOutMs.HasValue ? TimeSpan.FromMilliseconds(timeOutMs.Value) : null; - const string ChildProcessWaiting = "Waiting for the command to be cancelled"; const int CancelledExitCode = 42; const int ForceTerminationCode = 130; @@ -170,16 +168,14 @@ public async Task CancelOnProcessTermination_null_timeout_on_cancel_processing(i processExited.Should().Be(true); // Verify the process exit code - process.ExitCode.Should().Be(timeOutMs.HasValue ? ForceTerminationCode : CancelledExitCode); + process.ExitCode.Should().Be(CancelledExitCode); } [LinuxOnlyTheory] - [InlineData(100, SIGINT)] - [InlineData(100, SIGTERM)] - public async Task CancelOnProcessTermination_timeout_on_cancel_processing(int? timeOutMs, int signo) + [InlineData(SIGINT)] + [InlineData(SIGTERM)] + public async Task CancelOnProcessTermination_timeout_on_cancel_processing(int signo) { - TimeSpan? timeOut = timeOutMs.HasValue ? TimeSpan.FromMilliseconds(timeOutMs.Value) : null; - const string ChildProcessWaiting = "Waiting for the command to be cancelled"; const int CancelledExitCode = 42; const int ForceTerminationCode = 130; @@ -257,7 +253,7 @@ public async Task CancelOnProcessTermination_timeout_on_cancel_processing(int? t processExited.Should().Be(true); // Verify the process exit code - process.ExitCode.Should().Be(timeOutMs.HasValue ? ForceTerminationCode : CancelledExitCode); + process.ExitCode.Should().Be(ForceTerminationCode); } [DllImport("libc", SetLastError = true)] diff --git a/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs b/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs index 84862d475b..40cc593b75 100644 --- a/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs +++ b/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs @@ -43,7 +43,7 @@ public static class CommandLineBuilderExtensions /// Enables signaling and handling of process termination via a that can be passed to a during invocation. /// /// A command line builder. - /// + /// /// Optional timeout for the command to process the exit cancellation. /// If not passed, or passed null or non-positive timeout (including ), no timeout is enforced. /// If positive value is passed - command is forcefully terminated after the timeout with exit code 130 (as if was not called). @@ -52,14 +52,14 @@ public static class CommandLineBuilderExtensions /// The same instance of . public static CommandLineBuilder CancelOnProcessTermination( this CommandLineBuilder builder, - TimeSpan? cancelationProcessingTimeout = null) + TimeSpan? timeout = null) { // https://tldp.org/LDP/abs/html/exitcodes.html - 130 - script terminated by ctrl-c const int SIGINT_EXIT_CODE = 130; - if (cancelationProcessingTimeout == null || cancelationProcessingTimeout.Value < TimeSpan.Zero) + if (timeout == null || timeout.Value < TimeSpan.Zero) { - cancelationProcessingTimeout = Timeout.InfiniteTimeSpan; + timeout = Timeout.InfiniteTimeSpan; } builder.AddMiddleware(async (context, next) => @@ -78,15 +78,15 @@ public static CommandLineBuilder CancelOnProcessTermination( processExitHandler = (_, _) => { // Cancel asynchronously not to block the handler (as then the process might possibly run longer then what was the requested timeout) - Task timeoutTask = Task.Delay(cancelationProcessingTimeout.Value); + Task timeoutTask = Task.Delay(timeout.Value); Task cancelTask = Task.Factory.StartNew(cts.Cancel); // The process exits as soon as the event handler returns. // We provide a return value using Environment.ExitCode // because Main will not finish executing. // Wait for the invocation to finish. - if (!blockProcessExit.Wait(cancelationProcessingTimeout > TimeSpan.Zero - ? cancelationProcessingTimeout.Value + if (!blockProcessExit.Wait(timeout > TimeSpan.Zero + ? timeout.Value : Timeout.InfiniteTimeSpan)) { context.ExitCode = SIGINT_EXIT_CODE; @@ -108,10 +108,10 @@ public static CommandLineBuilder CancelOnProcessTermination( // If timeout was requested - make sure cancellation processing (or any other activity within the current process) // doesn't keep the process running after the timeout - if (cancelationProcessingTimeout! > TimeSpan.Zero) + if (timeout! > TimeSpan.Zero) { Task - .Delay(cancelationProcessingTimeout.Value, default) + .Delay(timeout.Value, default) .ContinueWith(t => { // Prevent our ProcessExit from intervene and block the exit From aba07e45f5de21e2bce9e05d3af9449dbaca4794 Mon Sep 17 00:00:00 2001 From: Jan Krivanek Date: Wed, 8 Jun 2022 14:55:17 +0200 Subject: [PATCH 5/6] Remove unused variable --- .../Invocation/CancelOnProcessTerminationTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs index 0ca15f5d5c..5e2b5713d0 100644 --- a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs +++ b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs @@ -98,7 +98,6 @@ public async Task CancelOnProcessTermination_null_timeout_on_cancel_processing(i { const string ChildProcessWaiting = "Waiting for the command to be cancelled"; const int CancelledExitCode = 42; - const int ForceTerminationCode = 130; Func> childProgram = (string[] args) => { From dfd788e011aa5ec97024ef184b1299a3b78f2f9b Mon Sep 17 00:00:00 2001 From: Jan Krivanek Date: Wed, 15 Jun 2022 09:25:30 +0200 Subject: [PATCH 6/6] Rename test cases to describe the scenarios --- .../Invocation/CancelOnProcessTerminationTests.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs index 5e2b5713d0..356b805a42 100644 --- a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs +++ b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs @@ -22,7 +22,7 @@ public class CancelOnProcessTerminationTests [LinuxOnlyTheory] [InlineData(SIGINT/*, Skip = "https://github.com/dotnet/command-line-api/issues/1206"*/)] // Console.CancelKeyPress [InlineData(SIGTERM)] // AppDomain.CurrentDomain.ProcessExit - public async Task CancelOnProcessTermination_cancels_on_process_termination(int signo) + public async Task CancelOnProcessTermination_provides_CancellationToken_that_signals_termination_when_no_timeout_is_specified(int signo) { const string ChildProcessWaiting = "Waiting for the command to be cancelled"; const int CancelledExitCode = 42; @@ -94,7 +94,7 @@ public async Task CancelOnProcessTermination_cancels_on_process_termination(int [LinuxOnlyTheory] [InlineData(SIGINT)] [InlineData(SIGTERM)] - public async Task CancelOnProcessTermination_null_timeout_on_cancel_processing(int signo) + public async Task CancelOnProcessTermination_provides_CancellationToken_that_signals_termination_when_null_timeout_is_specified(int signo) { const string ChildProcessWaiting = "Waiting for the command to be cancelled"; const int CancelledExitCode = 42; @@ -173,7 +173,7 @@ public async Task CancelOnProcessTermination_null_timeout_on_cancel_processing(i [LinuxOnlyTheory] [InlineData(SIGINT)] [InlineData(SIGTERM)] - public async Task CancelOnProcessTermination_timeout_on_cancel_processing(int signo) + public async Task CancelOnProcessTermination_provides_CancellationToken_that_signals_termination_and_execution_is_terminated_at_the_specified_timeout(int signo) { const string ChildProcessWaiting = "Waiting for the command to be cancelled"; const int CancelledExitCode = 42;