Skip to content

Commit de20229

Browse files
JanKrivanekJanKrivanek
authored
Add support for timeout in process termination handling (#1756)
* Add support for timeout in process termination handling * Fix tests - do not rely on captured closure * Improve termination handling to prevent (and test) sync blocking in Cancellation handler * Apply PR comments * Remove unused variable * Rename test cases to describe the scenarios
1 parent eaad234 commit de20229

File tree

3 files changed

+229
-14
lines changed

3 files changed

+229
-14
lines changed

‎src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,7 @@ System.CommandLine.Builder
240240
public static class CommandLineBuilderExtensions
241241
public static CommandLineBuilder AddMiddleware(this CommandLineBuilder builder, System.CommandLine.Invocation.InvocationMiddleware middleware, System.CommandLine.Invocation.MiddlewareOrder order = Default)
242242
public static CommandLineBuilder AddMiddleware(this CommandLineBuilder builder, System.Action<System.CommandLine.Invocation.InvocationContext> onInvoke, System.CommandLine.Invocation.MiddlewareOrder order = Default)
243-
public static CommandLineBuilder CancelOnProcessTermination(this CommandLineBuilder builder)
243+
public static CommandLineBuilder CancelOnProcessTermination(this CommandLineBuilder builder, System.Nullable<System.TimeSpan> timeout = null)
244244
public static CommandLineBuilder EnableDirectives(this CommandLineBuilder builder, System.Boolean value = True)
245245
public static CommandLineBuilder EnableLegacyDoubleDashBehavior(this CommandLineBuilder builder, System.Boolean value = True)
246246
public static CommandLineBuilder EnablePosixBundling(this CommandLineBuilder builder, System.Boolean value = True)

‎src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs

Lines changed: 166 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,9 @@ public class CancelOnProcessTerminationTests
2020
private const int SIGTERM = 15;
2121

2222
[LinuxOnlyTheory]
23-
[InlineData(SIGINT, Skip = "https://github.com/dotnet/command-line-api/issues/1206")] // Console.CancelKeyPress
23+
[InlineData(SIGINT/*, Skip = "https://github.com/dotnet/command-line-api/issues/1206"*/)] // Console.CancelKeyPress
2424
[InlineData(SIGTERM)] // AppDomain.CurrentDomain.ProcessExit
25-
public async Task CancelOnProcessTermination_cancels_on_process_termination(int signo)
25+
public async Task CancelOnProcessTermination_provides_CancellationToken_that_signals_termination_when_no_timeout_is_specified(int signo)
2626
{
2727
const string ChildProcessWaiting = "Waiting for the command to be cancelled";
2828
const int CancelledExitCode = 42;
@@ -91,6 +91,170 @@ public async Task CancelOnProcessTermination_cancels_on_process_termination(int
9191
process.ExitCode.Should().Be(CancelledExitCode);
9292
}
9393

94+
[LinuxOnlyTheory]
95+
[InlineData(SIGINT)]
96+
[InlineData(SIGTERM)]
97+
public async Task CancelOnProcessTermination_provides_CancellationToken_that_signals_termination_when_null_timeout_is_specified(int signo)
98+
{
99+
const string ChildProcessWaiting = "Waiting for the command to be cancelled";
100+
const int CancelledExitCode = 42;
101+
102+
Func<string[], Task<int>> childProgram = (string[] args) =>
103+
{
104+
var command = new Command("the-command");
105+
106+
command.SetHandler(async context =>
107+
{
108+
var cancellationToken = context.GetCancellationToken();
109+
110+
try
111+
{
112+
context.Console.WriteLine(ChildProcessWaiting);
113+
await Task.Delay(int.MaxValue, cancellationToken);
114+
context.ExitCode = 1;
115+
}
116+
catch (OperationCanceledException)
117+
{
118+
// For Process.Exit handling the event must remain blocked as long as the
119+
// command is executed.
120+
// We are currently blocking that event because CancellationTokenSource.Cancel
121+
// is called from the event handler.
122+
// We'll do an async Yield now. This means the Cancel call will return
123+
// and we're no longer actively blocking the event.
124+
// The event handler is responsible to continue blocking until the command
125+
// has finished executing. If it doesn't we won't get the CancelledExitCode.
126+
await Task.Yield();
127+
128+
// Exit code gets set here - but then execution continues and is let run till code voluntarily returns
129+
// hence exit code gets overwritten below
130+
context.ExitCode = 123;
131+
}
132+
133+
// This is an example of bad pattern and reason why we need a timeout on termination processing
134+
await Task.Delay(TimeSpan.FromMilliseconds(200));
135+
136+
context.ExitCode = CancelledExitCode;
137+
});
138+
139+
return new CommandLineBuilder(new RootCommand
140+
{
141+
command
142+
})
143+
// Unfortunately we cannot use test parameter here - RemoteExecutor currently doesn't capture the closure
144+
.CancelOnProcessTermination(null)
145+
.Build()
146+
.InvokeAsync("the-command");
147+
};
148+
149+
using RemoteExecution program = RemoteExecutor.Execute(childProgram, psi: new ProcessStartInfo { RedirectStandardOutput = true });
150+
151+
Process process = program.Process;
152+
153+
// Wait for the child to be in the command handler.
154+
string childState = await process.StandardOutput.ReadLineAsync();
155+
childState.Should().Be(ChildProcessWaiting);
156+
157+
// Request termination
158+
kill(process.Id, signo).Should().Be(0);
159+
160+
// Verify the process terminates timely
161+
bool processExited = process.WaitForExit(10000);
162+
if (!processExited)
163+
{
164+
process.Kill();
165+
process.WaitForExit();
166+
}
167+
processExited.Should().Be(true);
168+
169+
// Verify the process exit code
170+
process.ExitCode.Should().Be(CancelledExitCode);
171+
}
172+
173+
[LinuxOnlyTheory]
174+
[InlineData(SIGINT)]
175+
[InlineData(SIGTERM)]
176+
public async Task CancelOnProcessTermination_provides_CancellationToken_that_signals_termination_and_execution_is_terminated_at_the_specified_timeout(int signo)
177+
{
178+
const string ChildProcessWaiting = "Waiting for the command to be cancelled";
179+
const int CancelledExitCode = 42;
180+
const int ForceTerminationCode = 130;
181+
182+
Func<string[], Task<int>> childProgram = (string[] args) =>
183+
{
184+
var command = new Command("the-command");
185+
186+
command.SetHandler(async context =>
187+
{
188+
var cancellationToken = context.GetCancellationToken();
189+
190+
try
191+
{
192+
context.Console.WriteLine(ChildProcessWaiting);
193+
await Task.Delay(int.MaxValue, cancellationToken);
194+
context.ExitCode = 1;
195+
}
196+
catch (OperationCanceledException)
197+
{
198+
// For Process.Exit handling the event must remain blocked as long as the
199+
// command is executed.
200+
// We are currently blocking that event because CancellationTokenSource.Cancel
201+
// is called from the event handler.
202+
// We'll do an async Yield now. This means the Cancel call will return
203+
// and we're no longer actively blocking the event.
204+
// The event handler is responsible to continue blocking until the command
205+
// has finished executing. If it doesn't we won't get the CancelledExitCode.
206+
await Task.Yield();
207+
208+
context.ExitCode = CancelledExitCode;
209+
210+
// This is an example of bad pattern and reason why we need a timeout on termination processing
211+
await Task.Delay(TimeSpan.FromMilliseconds(1000));
212+
213+
// Execution should newer get here as termination processing has a timeout of 100ms
214+
Environment.Exit(123);
215+
}
216+
217+
// This is an example of bad pattern and reason why we need a timeout on termination processing
218+
await Task.Delay(TimeSpan.FromMilliseconds(1000));
219+
220+
// Execution should newer get here as termination processing has a timeout of 100ms
221+
Environment.Exit(123);
222+
});
223+
224+
return new CommandLineBuilder(new RootCommand
225+
{
226+
command
227+
})
228+
// Unfortunately we cannot use test parameter here - RemoteExecutor currently doesn't capture the closure
229+
.CancelOnProcessTermination(TimeSpan.FromMilliseconds(100))
230+
.Build()
231+
.InvokeAsync("the-command");
232+
};
233+
234+
using RemoteExecution program = RemoteExecutor.Execute(childProgram, psi: new ProcessStartInfo { RedirectStandardOutput = true });
235+
236+
Process process = program.Process;
237+
238+
// Wait for the child to be in the command handler.
239+
string childState = await process.StandardOutput.ReadLineAsync();
240+
childState.Should().Be(ChildProcessWaiting);
241+
242+
// Request termination
243+
kill(process.Id, signo).Should().Be(0);
244+
245+
// Verify the process terminates timely
246+
bool processExited = process.WaitForExit(10000);
247+
if (!processExited)
248+
{
249+
process.Kill();
250+
process.WaitForExit();
251+
}
252+
processExited.Should().Be(true);
253+
254+
// Verify the process exit code
255+
process.ExitCode.Should().Be(ForceTerminationCode);
256+
}
257+
94258
[DllImport("libc", SetLastError = true)]
95259
private static extern int kill(int pid, int sig);
96260
}

‎src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs

Lines changed: 62 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
using System.Linq;
1111
using System.Reflection;
1212
using System.Threading;
13+
using System.Threading.Tasks;
1314
using static System.Environment;
1415
using Process = System.CommandLine.Invocation.Process;
1516

@@ -42,9 +43,25 @@ public static class CommandLineBuilderExtensions
4243
/// Enables signaling and handling of process termination via a <see cref="CancellationToken"/> that can be passed to a <see cref="ICommandHandler"/> during invocation.
4344
/// </summary>
4445
/// <param name="builder">A command line builder.</param>
46+
/// <param name="timeout">
47+
/// Optional timeout for the command to process the exit cancellation.
48+
/// If not passed, or passed null or non-positive timeout (including <see cref="Timeout.InfiniteTimeSpan"/>), no timeout is enforced.
49+
/// If positive value is passed - command is forcefully terminated after the timeout with exit code 130 (as if <see cref="CancelOnProcessTermination"/> was not called).
50+
/// 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.
51+
/// </param>
4552
/// <returns>The same instance of <see cref="CommandLineBuilder"/>.</returns>
46-
public static CommandLineBuilder CancelOnProcessTermination(this CommandLineBuilder builder)
53+
public static CommandLineBuilder CancelOnProcessTermination(
54+
this CommandLineBuilder builder,
55+
TimeSpan? timeout = null)
4756
{
57+
// https://tldp.org/LDP/abs/html/exitcodes.html - 130 - script terminated by ctrl-c
58+
const int SIGINT_EXIT_CODE = 130;
59+
60+
if (timeout == null || timeout.Value < TimeSpan.Zero)
61+
{
62+
timeout = Timeout.InfiniteTimeSpan;
63+
}
64+
4865
builder.AddMiddleware(async (context, next) =>
4966
{
5067
bool cancellationHandlingAdded = false;
@@ -56,24 +73,58 @@ public static CommandLineBuilder CancelOnProcessTermination(this CommandLineBuil
5673
{
5774
blockProcessExit = new ManualResetEventSlim(initialState: false);
5875
cancellationHandlingAdded = true;
59-
consoleHandler = (_, args) =>
60-
{
61-
cts.Cancel();
62-
// Stop the process from terminating.
63-
// Since the context was cancelled, the invocation should
64-
// finish and Main will return.
65-
args.Cancel = true;
66-
};
76+
// Default limit for ProcesExit handler is 2 seconds
77+
// https://docs.microsoft.com/en-us/dotnet/api/system.appdomain.processexit?view=net-6.0
6778
processExitHandler = (_, _) =>
6879
{
69-
cts.Cancel();
80+
// Cancel asynchronously not to block the handler (as then the process might possibly run longer then what was the requested timeout)
81+
Task timeoutTask = Task.Delay(timeout.Value);
82+
Task cancelTask = Task.Factory.StartNew(cts.Cancel);
83+
7084
// The process exits as soon as the event handler returns.
7185
// We provide a return value using Environment.ExitCode
7286
// because Main will not finish executing.
7387
// Wait for the invocation to finish.
74-
blockProcessExit.Wait();
88+
if (!blockProcessExit.Wait(timeout > TimeSpan.Zero
89+
? timeout.Value
90+
: Timeout.InfiniteTimeSpan))
91+
{
92+
context.ExitCode = SIGINT_EXIT_CODE;
93+
}
94+
// 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)
95+
else if (Task.WaitAny(timeoutTask, cancelTask) == 0)
96+
{
97+
// The async cancellation didn't finish in timely manner
98+
context.ExitCode = SIGINT_EXIT_CODE;
99+
}
75100
ExitCode = context.ExitCode;
76101
};
102+
consoleHandler = (_, args) =>
103+
{
104+
// Stop the process from terminating.
105+
// Since the context was cancelled, the invocation should
106+
// finish and Main will return.
107+
args.Cancel = true;
108+
109+
// If timeout was requested - make sure cancellation processing (or any other activity within the current process)
110+
// doesn't keep the process running after the timeout
111+
if (timeout! > TimeSpan.Zero)
112+
{
113+
Task
114+
.Delay(timeout.Value, default)
115+
.ContinueWith(t =>
116+
{
117+
// Prevent our ProcessExit from intervene and block the exit
118+
AppDomain.CurrentDomain.ProcessExit -= processExitHandler;
119+
Environment.Exit(SIGINT_EXIT_CODE);
120+
}, (CancellationToken)default);
121+
}
122+
123+
// Cancel synchronously here - no need to perform it asynchronously as the timeout is already running (and would kill the process if needed),
124+
// plus we cannot wait only on the cancellation (e.g. via `Task.Factory.StartNew(cts.Cancel).Wait(cancelationProcessingTimeout.Value)`)
125+
// as we need to abort any other possible execution within the process - even outside the context of cancellation processing
126+
cts.Cancel();
127+
};
77128
Console.CancelKeyPress += consoleHandler;
78129
AppDomain.CurrentDomain.ProcessExit += processExitHandler;
79130
};

0 commit comments

Comments
 (0)