-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Add Process.StartAndForget APIs for fire-and-forget process launching #126078
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?
Changes from all commits
ad79bd9
0f8be81
9be6c1c
bff4ded
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.Collections.Generic; | ||
| using System.Runtime.Versioning; | ||
|
|
||
| namespace System.Diagnostics | ||
| { | ||
| public partial class Process | ||
| { | ||
| /// <summary> | ||
| /// Starts the process described by <paramref name="startInfo"/>, captures its process ID, | ||
| /// releases all associated resources, and returns the process ID. | ||
| /// </summary> | ||
| /// <param name="startInfo">The <see cref="ProcessStartInfo"/> that contains the information used to start the process.</param> | ||
| /// <returns>The process ID of the started process.</returns> | ||
| /// <exception cref="ArgumentNullException"><paramref name="startInfo"/> is <see langword="null"/>.</exception> | ||
| /// <exception cref="InvalidOperationException"> | ||
| /// One or more of <see cref="ProcessStartInfo.RedirectStandardInput"/>, | ||
| /// <see cref="ProcessStartInfo.RedirectStandardOutput"/>, or | ||
| /// <see cref="ProcessStartInfo.RedirectStandardError"/> is set to <see langword="true"/>. | ||
| /// Stream redirection is not supported in fire-and-forget scenarios because redirected streams | ||
| /// must be drained to avoid deadlocks. | ||
| /// </exception> | ||
| /// <remarks> | ||
| /// <para> | ||
| /// This method is designed for fire-and-forget scenarios where the caller wants to launch a process | ||
| /// and does not need to interact with it further. It starts the process, captures its process ID, | ||
| /// disposes the <see cref="Process"/> instance to release all associated resources, and returns the | ||
| /// process ID. The started process continues to run independently. | ||
| /// </para> | ||
| /// <para> | ||
| /// Calling this method ensures proper resource cleanup on the caller's side: unlike calling | ||
| /// <see cref="Start(ProcessStartInfo)"/> and discarding the returned object, this method guarantees | ||
| /// that the underlying operating-system resources held by the <see cref="Process"/> object are | ||
| /// released promptly. | ||
| /// </para> | ||
| /// </remarks> | ||
| [UnsupportedOSPlatform("ios")] | ||
| [UnsupportedOSPlatform("tvos")] | ||
| [SupportedOSPlatform("maccatalyst")] | ||
| public static int StartAndForget(ProcessStartInfo startInfo) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(startInfo); | ||
|
|
||
| if (startInfo.RedirectStandardInput || startInfo.RedirectStandardOutput || startInfo.RedirectStandardError) | ||
| { | ||
| throw new InvalidOperationException(SR.StartAndForget_RedirectNotSupported); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to throw? Presumably if redirection is set, the user is really just asking for it to not go to the default stdin / stdout / stderr? In which case, we could make these convenient by just setting these up to be null handles, handling the disposal of those, etc.?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My take is following: so far, I would prefer to throw for |
||
| } | ||
|
|
||
| using Process process = new Process(); | ||
| process.StartInfo = startInfo; | ||
| process.Start(); | ||
| return process.Id; | ||
|
Comment on lines
+51
to
+54
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't we want to use the SafeProcessHandle.Start functionality here?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, as soon as we have And FWIW I want to expose it as soon as #125848 gets merged (just move it from Process.StartCore as is to SafeProcessHandle.Start |
||
| } | ||
|
|
||
| /// <summary> | ||
| /// Starts a process with the specified file name and optional arguments, captures its process ID, | ||
| /// releases all associated resources, and returns the process ID. | ||
| /// </summary> | ||
| /// <param name="fileName">The name of the application or document to start.</param> | ||
| /// <param name="arguments"> | ||
| /// The command-line arguments to pass to the process. Pass <see langword="null"/> or an empty list | ||
| /// to start the process without additional arguments. | ||
| /// </param> | ||
| /// <returns>The process ID of the started process.</returns> | ||
| /// <exception cref="ArgumentNullException"><paramref name="fileName"/> is <see langword="null"/>.</exception> | ||
| /// <remarks> | ||
| /// <para> | ||
| /// This method is designed for fire-and-forget scenarios where the caller wants to launch a process | ||
| /// and does not need to interact with it further. It starts the process, captures its process ID, | ||
| /// disposes the <see cref="Process"/> instance to release all associated resources, and returns the | ||
| /// process ID. The started process continues to run independently. | ||
| /// </para> | ||
| /// <para> | ||
| /// Calling this method ensures proper resource cleanup on the caller's side: unlike calling | ||
| /// <see cref="Start(string)"/> and discarding the returned object, this method guarantees that the | ||
| /// underlying operating-system resources held by the <see cref="Process"/> object are released | ||
| /// promptly. | ||
| /// </para> | ||
| /// </remarks> | ||
| [UnsupportedOSPlatform("ios")] | ||
| [UnsupportedOSPlatform("tvos")] | ||
| [SupportedOSPlatform("maccatalyst")] | ||
| public static int StartAndForget(string fileName, IList<string>? arguments = null) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(fileName); | ||
|
|
||
| ProcessStartInfo startInfo = new ProcessStartInfo(fileName); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stephentoub triggered by your comment on the I assume this isn't using the constructor that accepts an Would it be an option to add an overload like: Or might that cause compatibility issues?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, should have been: @adamsitnik may be we can add an |
||
| if (arguments is not null) | ||
| { | ||
| foreach (string argument in arguments) | ||
| { | ||
| startInfo.ArgumentList.Add(argument); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eiriktsarpalis / @tannergooding, this kind of loop comes up all the time. We should really have an AddRange for IList. We've held off on adding it as an extension method because of the possibility of instead adding it as a DIM, and decisions there have been blocked on the whole I{ReadOnly}List consolidation, but it'd be a shame if we delayed getting AddRange even further. Can we come up with a decision for how to add AddRange regardless of the I{ReadOnly}List decision? |
||
| } | ||
|
|
||
| return StartAndForget(startInfo); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.Collections.Generic; | ||
| using Microsoft.DotNet.RemoteExecutor; | ||
| using Xunit; | ||
|
|
||
| namespace System.Diagnostics.Tests | ||
| { | ||
| public class StartAndForgetTests : ProcessTestBase | ||
| { | ||
| [ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] | ||
| [InlineData(true)] | ||
| [InlineData(false)] | ||
| public void StartAndForget_StartsProcessAndReturnsValidPid(bool useProcessStartInfo) | ||
| { | ||
| Process template = CreateProcessLong(); | ||
| int pid = useProcessStartInfo | ||
| ? Process.StartAndForget(template.StartInfo) | ||
| : Process.StartAndForget(template.StartInfo.FileName, template.StartInfo.ArgumentList); | ||
|
|
||
| Assert.True(pid > 0); | ||
|
|
||
| using Process launched = Process.GetProcessById(pid); | ||
| try | ||
| { | ||
| Assert.False(launched.HasExited); | ||
| } | ||
| finally | ||
| { | ||
| launched.Kill(); | ||
| launched.WaitForExit(); | ||
| } | ||
| } | ||
|
|
||
| [Fact] | ||
| public void StartAndForget_WithNullArguments_StartsProcess() | ||
| { | ||
| // hostname is not available on Android or Azure Linux. | ||
| // ls is available on every Unix. | ||
| int pid = Process.StartAndForget(OperatingSystem.IsWindows() ? "hostname" : "ls", null); | ||
|
|
||
| Assert.True(pid > 0); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void StartAndForget_WithStartInfo_NullStartInfo_ThrowsArgumentNullException() | ||
| { | ||
| AssertExtensions.Throws<ArgumentNullException>("startInfo", () => Process.StartAndForget((ProcessStartInfo)null!)); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void StartAndForget_WithFileName_NullFileName_ThrowsArgumentNullException() | ||
| { | ||
| AssertExtensions.Throws<ArgumentNullException>("fileName", () => Process.StartAndForget((string)null!)); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData(true, false, false)] | ||
| [InlineData(false, true, false)] | ||
| [InlineData(false, false, true)] | ||
| public void StartAndForget_WithRedirectedStreams_ThrowsInvalidOperationException( | ||
| bool redirectInput, bool redirectOutput, bool redirectError) | ||
| { | ||
| ProcessStartInfo startInfo = new("someprocess") | ||
| { | ||
| RedirectStandardInput = redirectInput, | ||
| RedirectStandardOutput = redirectOutput, | ||
| RedirectStandardError = redirectError, | ||
| }; | ||
|
|
||
| Assert.Throws<InvalidOperationException>(() => Process.StartAndForget(startInfo)); | ||
| } | ||
| } | ||
| } |
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.
What would someone be trying to do with stream redirection? I wonder if it makes sense to nudge folks towards setting StandardXxHandle here, possible to OpenNullHandle?