Add Process.StartAndForget APIs for fire-and-forget process launching#126078
Add Process.StartAndForget APIs for fire-and-forget process launching#126078
Conversation
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com> Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/095d096c-d5d9-4ad1-b5f2-45fbadf30315
|
Tagging subscribers to this area: @dotnet/area-system-diagnostics-process |
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot address my feedback
src/libraries/System.Diagnostics.Process/ref/System.Diagnostics.Process.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/ref/System.Diagnostics.Process.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/StartAndForget.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/StartAndForget.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/StartAndForget.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/StartAndForget.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com> Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/942ffa7e-9504-40eb-9a58-9c4b6ceee3c1
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot address my feedback
src/libraries/System.Diagnostics.Process/tests/StartAndForget.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com> Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/71890478-dd77-4849-8278-bb3194c8489f
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot address my feedback
| public void StartAndForget_WithNullArguments_StartsProcess() | ||
| { | ||
| // hostname is available on all platforms and requires no arguments | ||
| int pid = Process.StartAndForget("hostname", null); |
There was a problem hiding this comment.
This test fails on Azure Linux:
System.Diagnostics.Tests.StartAndForgetTests.StartAndForget_WithNullArguments_StartsProcess [FAIL]
System.ComponentModel.Win32Exception : An error occurred trying to start process 'hostname' with working directory '/root/helix/work/workitem/e'. No such file or directory
Stack Trace:
/_/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs(561,0): at System.Diagnostics.Process.ForkAndExecProcess(ProcessStartInfo startInfo, String resolvedFilename, String[] argv, String[] envp, String cwd, Boolean setCredentials, UInt32 userId, UInt32 groupId, UInt32[] groups, Int32& stdinFd, Int32& stdoutFd, Int32& stderrFd, Boolean usesTerminal, Boolean throwOnNoExec)
/_/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs(456,0): at System.Diagnostics.Process.StartCore(ProcessStartInfo startInfo)
/_/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs(1284,0): at System.Diagnostics.Process.Start()
/_/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Scenarios.cs(53,0): at System.Diagnostics.Process.StartAndForget(ProcessStartInfo startInfo)
/_/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Scenarios.cs(98,0): at System.Diagnostics.Process.StartAndForget(String fileName, IList`1 arguments)
/_/src/libraries/System.Diagnostics.Process/tests/StartAndForget.cs(40,0): at System.Diagnostics.Tests.StartAndForgetTests.StartAndForget_WithNullArguments_StartsProcess()
/_/src/mono/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.Mono.cs(22,0): at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
/_/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.cs(57,0): at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
@copilot use "hostname" on Windows and "ls" on other OSes
| int pid = Process.StartAndForget("hostname", null); | |
| // hostname is not available on Android or Azure Linux. | |
| // ls is available on every Unix. | |
| int pid = Process.StartAndForget(OperatingSystem.IsWindows() ? "hostname" : "ls", null); |
There was a problem hiding this comment.
Fixed in bff4ded — applied the suggestion exactly: hostname on Windows, ls on Unix.
| <value>Invalid performance counter data with type '{0}'.</value> | ||
| </data> | ||
| <data name="StartAndForget_RedirectNotSupported" xml:space="preserve"> | ||
| <value>Stream redirection is not supported by StartAndForget. Redirected streams must be drained to avoid deadlocks, which is incompatible with fire-and-forget semantics.</value> |
There was a problem hiding this comment.
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?
| using Process process = new Process(); | ||
| process.StartInfo = startInfo; | ||
| process.Start(); | ||
| return process.Id; |
There was a problem hiding this comment.
Wouldn't we want to use the SafeProcessHandle.Start functionality here?
There was a problem hiding this comment.
Wouldn't we want to use the SafeProcessHandle.Start functionality here?
Yes, as soon as we have SafeProcessHandle.Start, this API should use it instead of Process.
And FWIW I want to expose it as soon as #125848 gets merged (just move it from Process.StartCore as is to SafeProcessHandle.Start
| foreach (string argument in arguments) | ||
| { | ||
| startInfo.ArgumentList.Add(argument); | ||
| } |
There was a problem hiding this comment.
@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?
|
|
||
| if (startInfo.RedirectStandardInput || startInfo.RedirectStandardOutput || startInfo.RedirectStandardError) | ||
| { | ||
| throw new InvalidOperationException(SR.StartAndForget_RedirectNotSupported); |
There was a problem hiding this comment.
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.?
There was a problem hiding this comment.
My take is following: so far, RedirectStandardX always meant "I want to interact with the X". With the new APIs, the users will be allowed to make it explicit (info.StandardXHandle = File.OpenNullHandle()); So I would prefer not to choose any deafult other than pipe when RedirectStandardX is set to true
I would prefer to throw for RedirectStandardX here, but I am supportive of setting all standard handles to NUL by default.
…s on Unix Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com> Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/a8a7e105-927e-424f-986b-434912ec8a8e
Adds two new static methods to
Processfor scenarios where you want to launch a process, get its PID, and immediately release all resources without waiting for it to exit.Description
New APIs (
Process.Scenarios.cs):Process.StartAndForget(ProcessStartInfo)— validates no stream redirection is configured (redirected streams must be drained to prevent deadlocks, incompatible with fire-and-forget), starts the process, captures the PID, disposes theProcessinstance, and returns the PID.Process.StartAndForget(string fileName, IList<string>? arguments = null)— builds aProcessStartInfoand delegates to theProcessStartInfooverload above.Redirection guard — throws
InvalidOperationExceptionif anyRedirectStandardInput/Output/Errorflag is set, with a message explaining the deadlock risk.Supporting changes:
// this needs to come after the ios attribute due to limitations in the platform analyzercomment on[SupportedOSPlatformAttribute("maccatalyst")], consistent with otherStartoverloads in the fileStartAndForget_RedirectNotSupportedinStrings.resxProcess.Scenarios.csadded toSystem.Diagnostics.Process.csprojStartAndForget.cs:[ConditionalTheory]parameterized bybool useProcessStartInfo, with explicitKill()/WaitForExit()cleanup intry/finallyArgumentNullExceptiontests useAssertExtensions.Throwsto verify parameter names ("startInfo"and"fileName")InvalidOperationExceptionfor each redirect flag combination via[Theory][Fact](noRemoteExecutordependency) that passesnulldirectly; useshostnameon Windows andlson Unix (hostname is not available on Android or Azure Linux)⚡ Quickly spin up Copilot coding agent tasks from anywhere on your macOS or Windows machine with Raycast.