Repository navigation
Make Process.Run methods kill the entire process tree on timeout or cancellation #136513
+226
−86
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
|
|
||
| using System.Collections.Generic; | ||
| using System.IO; | ||
| using System.Runtime.ExceptionServices; | ||
| using System.Runtime.Versioning; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
|
|
@@ -90,7 +91,7 @@ public static int StartAndForget(string fileName, IEnumerable<string>? arguments | |
| /// <param name="timeout"> | ||
| /// The maximum amount of time to wait for the process to exit. | ||
| /// When <see langword="null"/>, waits indefinitely. | ||
| /// If the process does not exit within the specified timeout, it and its descendants are killed. | ||
| /// </param> | ||
| /// <returns>The exit status of the process.</returns> | ||
| /// <exception cref="ArgumentNullException"><paramref name="startInfo"/> is <see langword="null"/>.</exception> | ||
|
|
@@ -106,11 +107,17 @@ public static ProcessExitStatus Run(ProcessStartInfo startInfo, TimeSpan? timeou | |
|
|
||
| ThrowIfUseShellExecute(startInfo, nameof(Run)); | ||
|
|
||
| startInfo.ThrowIfInvalid(out bool anyRedirection, out _); | ||
| if (anyRedirection) | ||
| { | ||
| throw new InvalidOperationException(SR.CantSetRedirectForSafeProcessHandleStart); | ||
| } | ||
|
|
||
| using Process process = Start(startInfo)!; | ||
|
|
||
| return timeout.HasValue | ||
| ? process.WaitForExitOrKillEntireProcessTreeOnTimeout(timeout.Value) | ||
| : process.WaitForExitStatus(); | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -129,7 +136,7 @@ public static ProcessExitStatus Run(ProcessStartInfo startInfo, TimeSpan? timeou | |
| /// <param name="timeout"> | ||
| /// The maximum amount of time to wait for the process to exit. | ||
| /// When <see langword="null"/>, waits indefinitely. | ||
| /// If the process does not exit within the specified timeout, it and its descendants are killed. | ||
| /// </param> | ||
| /// <returns>The exit status of the process.</returns> | ||
| /// <exception cref="ArgumentNullException"><paramref name="fileName"/> is <see langword="null"/>.</exception> | ||
|
|
@@ -155,7 +162,7 @@ public static ProcessExitStatus Run(string fileName, IEnumerable<string>? argume | |
| /// <param name="startInfo">The <see cref="ProcessStartInfo"/> that contains the information used to start the process.</param> | ||
| /// <param name="cancellationToken"> | ||
| /// A token to cancel the asynchronous operation. | ||
| /// If the token is canceled, the process and its descendants are killed. | ||
| /// </param> | ||
| /// <returns>A task that represents the asynchronous operation. The value of the task contains the exit status of the process.</returns> | ||
| /// <exception cref="ArgumentNullException"><paramref name="startInfo"/> is <see langword="null"/>.</exception> | ||
|
|
@@ -171,9 +178,14 @@ public static async Task<ProcessExitStatus> RunAsync(ProcessStartInfo startInfo, | |
|
|
||
| ThrowIfUseShellExecute(startInfo, nameof(RunAsync)); | ||
|
|
||
| startInfo.ThrowIfInvalid(out bool anyRedirection, out _); | ||
| if (anyRedirection) | ||
| { | ||
| throw new InvalidOperationException(SR.CantSetRedirectForSafeProcessHandleStart); | ||
| } | ||
|
|
||
| using Process process = Start(startInfo)!; | ||
| return await process.WaitForExitOrKillEntireProcessTreeOnCancellationAsync(cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -191,7 +203,7 @@ public static async Task<ProcessExitStatus> RunAsync(ProcessStartInfo startInfo, | |
| /// </param> | ||
| /// <param name="cancellationToken"> | ||
| /// A token to cancel the asynchronous operation. | ||
| /// If the token is canceled, the process and its descendants are killed. | ||
| /// </param> | ||
| /// <returns>A task that represents the asynchronous operation. The value of the task contains the exit status of the process.</returns> | ||
| /// <exception cref="ArgumentNullException"><paramref name="fileName"/> is <see langword="null"/>.</exception> | ||
|
|
@@ -219,7 +231,7 @@ public static async Task<ProcessExitStatus> RunAsync(string fileName, IEnumerabl | |
| /// <param name="timeout"> | ||
| /// The maximum amount of time to wait for the process to exit. | ||
| /// When <see langword="null"/>, waits indefinitely. | ||
| /// If the process does not exit within the specified timeout, it and its descendants are killed. | ||
| /// </param> | ||
| /// <returns>The captured text output and exit status of the process.</returns> | ||
| /// <exception cref="ArgumentNullException"><paramref name="startInfo"/> is <see langword="null"/>.</exception> | ||
|
|
@@ -249,7 +261,8 @@ public static ProcessTextOutput RunAndCaptureText(ProcessStartInfo startInfo, Ti | |
| } | ||
| catch | ||
| { | ||
| // Cleanup must not replace the original capture exception. | ||
| try { process.TryKillEntireTree(); } catch { } | ||
| throw; | ||
| } | ||
|
|
||
|
|
@@ -259,11 +272,11 @@ public static ProcessTextOutput RunAndCaptureText(ProcessStartInfo startInfo, Ti | |
| TimeSpan elapsed = Stopwatch.GetElapsedTime(startTimestamp); | ||
| TimeSpan remaining = timeout.Value - elapsed; | ||
| remaining = remaining >= TimeSpan.Zero ? remaining : TimeSpan.Zero; | ||
| exitStatus = process.WaitForExitOrKillEntireProcessTreeOnTimeout(remaining); | ||
| } | ||
| else | ||
| { | ||
| exitStatus = process.WaitForExitStatus(); | ||
| } | ||
|
|
||
| return new ProcessTextOutput(exitStatus, standardOutput, standardError, process.Id); | ||
|
|
@@ -281,7 +294,7 @@ public static ProcessTextOutput RunAndCaptureText(ProcessStartInfo startInfo, Ti | |
| /// <param name="timeout"> | ||
| /// The maximum amount of time to wait for the process to exit. | ||
| /// When <see langword="null"/>, waits indefinitely. | ||
| /// If the process does not exit within the specified timeout, it and its descendants are killed. | ||
| /// </param> | ||
| /// <returns>The captured text output and exit status of the process.</returns> | ||
| /// <exception cref="ArgumentNullException"><paramref name="fileName"/> is <see langword="null"/>.</exception> | ||
|
|
@@ -299,7 +312,7 @@ public static ProcessTextOutput RunAndCaptureText(string fileName, IEnumerable<s | |
| /// <param name="startInfo">The <see cref="ProcessStartInfo"/> that contains the information used to start the process.</param> | ||
| /// <param name="cancellationToken"> | ||
| /// A token to cancel the asynchronous operation. | ||
| /// If the token is canceled, the process and its descendants are killed. | ||
| /// </param> | ||
| /// <returns>A task that represents the asynchronous operation. The value of the task contains the captured text output and exit status of the process.</returns> | ||
| /// <exception cref="ArgumentNullException"><paramref name="startInfo"/> is <see langword="null"/>.</exception> | ||
|
|
@@ -327,11 +340,12 @@ public static async Task<ProcessTextOutput> RunAndCaptureTextAsync(ProcessStartI | |
| } | ||
| catch | ||
| { | ||
| // Cleanup must not replace the original capture exception. | ||
| try { process.TryKillEntireTree(); } catch { } | ||
| throw; | ||
| } | ||
|
|
||
| ProcessExitStatus exitStatus = await process.WaitForExitOrKillEntireProcessTreeOnCancellationAsync(cancellationToken).ConfigureAwait(false); | ||
|
|
||
| return new ProcessTextOutput(exitStatus, standardOutput, standardError, process.Id); | ||
| } | ||
|
|
@@ -347,7 +361,7 @@ public static async Task<ProcessTextOutput> RunAndCaptureTextAsync(ProcessStartI | |
| /// </param> | ||
| /// <param name="cancellationToken"> | ||
| /// A token to cancel the asynchronous operation. | ||
| /// If the token is canceled, the process and its descendants are killed. | ||
| /// </param> | ||
| /// <returns>A task that represents the asynchronous operation. The value of the task contains the captured text output and exit status of the process.</returns> | ||
| /// <exception cref="ArgumentNullException"><paramref name="fileName"/> is <see langword="null"/>.</exception> | ||
|
|
@@ -374,6 +388,45 @@ private static ProcessStartInfo CreateStartInfoForCapture(string fileName, IEnum | |
| return startInfo; | ||
| } | ||
|
|
||
| [UnsupportedOSPlatform("ios")] | ||
| [UnsupportedOSPlatform("tvos")] | ||
| [SupportedOSPlatform("maccatalyst")] | ||
| private ProcessExitStatus WaitForExitOrKillEntireProcessTreeOnTimeout(TimeSpan timeout) | ||
| { | ||
| SafeProcessHandle safeProcessHandle = _processHandle!; | ||
| if (safeProcessHandle.TryWaitForExit(timeout, out ProcessExitStatus? exitStatus)) | ||
|
adamsitnik marked this conversation as resolved.
Resolved
|
||
| { | ||
| return exitStatus; | ||
| } | ||
|
|
||
| safeProcessHandle.Canceled = true; | ||
| TryKillEntireTree(); | ||
|
|
||
| return safeProcessHandle.WaitForExit(); | ||
| } | ||
|
|
||
| [UnsupportedOSPlatform("ios")] | ||
| [UnsupportedOSPlatform("tvos")] | ||
| [SupportedOSPlatform("maccatalyst")] | ||
| private Task<ProcessExitStatus> WaitForExitOrKillEntireProcessTreeOnCancellationAsync(CancellationToken cancellationToken) | ||
| => _processHandle!.WaitForExitOrKillOnCancellationAsync(cancellationToken, cancellationToken.CanBeCanceled ? TryKillEntireTree : null); | ||
|
|
||
| [UnsupportedOSPlatform("ios")] | ||
| [UnsupportedOSPlatform("tvos")] | ||
| [SupportedOSPlatform("maccatalyst")] | ||
| private void TryKillEntireTree() | ||
| { | ||
| try | ||
| { | ||
| Kill(entireProcessTree: true); | ||
|
Member
There was a problem hiding this comment.
Member
Author
There was a problem hiding this comment.
As of today, it is. With #126273, we could make it trivial, especially here where we know that we are about to start a process and we know whether we may need to kill entire tree or not (timeout/cancelable token was provided). This is what some of our 1P customers do (I keep an eye on code that was not able to remove all custom p/invokes after migrating to the new APIs).
We are going to increase the size on disk, but on the other hand we are going to protect some of our customers from hard to diagnose issues. I hate regressing any kind of benchmarks, but here I think it's the right thing to do. Especially since we can eliminate this regression (in a follow up PR) and make the whole operation faster and atomic (at the cost of putting all processes into a Job).
Member
There was a problem hiding this comment.
We should be also asking how many are not doing this and would be actually broken by this change. This change can cut both ways - both on functionality and/or on performance. Are there example in other ecosystems where simple process APIs try to kill the whole process tree on failure?
Member
Author
There was a problem hiding this comment.
I think that 99.9% would benefit and only 0.1% could be broken for scenarios like spawning background compiler process for performance purposes.
I've used the Office Copilot Researcher to answer your quesiton: 
@jkotas which leads me to a conclusion that this most likely should be controlled via a new, optional bool parameter: ProcessExitStatus> RunAsync(
string fileName, IEnumerable<string>? arguments = null,
bool silent = false,
+ bool entireProcessTree = false,
CancellationToken ct = default)
Member
There was a problem hiding this comment.
Yes, it sounds like that to me as well. It is unfortunate that we have made If we do this, we should look at trimming the implementation to be leaner. |
||
| } | ||
| catch | ||
| { | ||
| // In case killing the process tree fails, attempt to Kill the root. | ||
| Kill(); | ||
| } | ||
| } | ||
|
|
||
| private static void ThrowIfUseShellExecute(ProcessStartInfo startInfo, string methodName) | ||
| { | ||
| if (startInfo.UseShellExecute) | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.