Add StandardInput/Output/Error SafeFileHandle properties and LeaveHandlesOpen to ProcessStartInfo#125848
Conversation
…artInfo Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
| { | ||
| if (startInfo.UseShellExecute) | ||
| { | ||
| throw new InvalidOperationException(SR.CantUseHandlesWithShellExecute); |
There was a problem hiding this comment.
Let's re-use existing error message:
| throw new InvalidOperationException(SR.CantUseHandlesWithShellExecute); | |
| throw new InvalidOperationException(SR.CantRedirectStreams); |
There was a problem hiding this comment.
Done in 60844f3 — now reuses SR.CantRedirectStreams and removed the unused CantUseHandlesWithShellExecute string from Strings.resx.
|
Tagging subscribers to this area: @dotnet/area-system-diagnostics-process |
| /// When set, the handle is passed directly to the child process and <see cref="RedirectStandardInput"/> must be <see langword="false"/>. | ||
| /// </summary> | ||
| /// <value>A <see cref="SafeFileHandle"/> to use as the standard input handle of the child process, or <see langword="null"/> to use the default behavior.</value> | ||
| public SafeFileHandle? StandardInput { get; set; } |
There was a problem hiding this comment.
Are there any requirements around its inheritability that should be called out in the docs?
Maybe add some XML comments remarks around when this would be used, what APIs it might typically used in conjunction with, etc.?
There was a problem hiding this comment.
Added <remarks> sections to all three properties in 60844f3 covering: inheritability requirement, typical APIs to use (SafeFileHandle.CreateAnonymousPipe, File.OpenHandle), interaction with the new LeaveHandlesOpen property, and constraints (RedirectStandard* / UseShellExecute).
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot please address all the feedback
src/libraries/System.Diagnostics.Process/ref/System.Diagnostics.Process.cs
Show resolved
Hide resolved
| using SafeFileHandle inputHandle = Console.OpenStandardInputHandle(); | ||
| using SafeFileHandle outputHandle = Console.OpenStandardOutputHandle(); | ||
| using SafeFileHandle errorHandle = Console.OpenStandardErrorHandle(); | ||
|
|
||
| startInfo.StandardInput = inputHandle; | ||
| startInfo.StandardOutput = outputHandle; | ||
| startInfo.StandardError = errorHandle; |
There was a problem hiding this comment.
Let the new property dispose all these handles for us.
| using SafeFileHandle inputHandle = Console.OpenStandardInputHandle(); | |
| using SafeFileHandle outputHandle = Console.OpenStandardOutputHandle(); | |
| using SafeFileHandle errorHandle = Console.OpenStandardErrorHandle(); | |
| startInfo.StandardInput = inputHandle; | |
| startInfo.StandardOutput = outputHandle; | |
| startInfo.StandardError = errorHandle; | |
| startInfo.StandardInput = Console.OpenStandardInputHandle(); | |
| startInfo.StandardOutput = Console.OpenStandardOutputHandle(); | |
| startInfo.StandardError = Console.OpenStandardErrorHandle(); |
There was a problem hiding this comment.
Done in 60844f3 — removed the using statements; handles are now disposed by Process.Start since LeaveHandlesOpen defaults to false.
…pen property, improve XML docs Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com> Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/9f61c68e-edfd-42e9-b86b-bfb9ee7d8946
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot address my feedback
| /// </summary> | ||
| /// <remarks> | ||
| /// <para> | ||
| /// The handle must be inheritable. Use <see cref="SafeFileHandle.CreateAnonymousPipe"/> to create a pair of |
There was a problem hiding this comment.
It should not be inheritable, because other process may inherit it by accident before we get here.
Also, do mention File.OpenNullHandle and Console.OpenSatndardInputHandle API
There was a problem hiding this comment.
Updated in c8b8723 — the docs now state the handle does not need to be inheritable (runtime handles that). Also added mentions of File.OpenNullHandle and Console.OpenStandardInputHandle / OpenStandardOutputHandle / OpenStandardErrorHandle for each respective property.
| using Process process = Process.Start(startInfo)!; | ||
|
|
||
| // Close the parent copy of the child handle, so the pipe will signal EOF when the child exits | ||
| writePipe.Close(); |
There was a problem hiding this comment.
Removed in c8b8723 — Process.Start closes it now since LeaveHandlesOpen defaults to false.
|
|
||
| // Close the parent copy of the child handles, so the pipes will signal EOF when the child exits | ||
| outputWrite.Close(); | ||
| errorWrite.Close(); |
…ullHandle/OpenStandard*Handle APIs, remove redundant Close() calls Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com> Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/29b43a66-dcf9-47b0-95df-28d834129305
Description
Adds three new
SafeFileHandle?properties and aLeaveHandlesOpenproperty toProcessStartInfothat allow passing file handles directly to child processes, bypassing theRedirectStandard*pipe-based mechanism.By default,
Process.Startdisposes the standard handles after starting the child process. SetLeaveHandlesOpentotrueto keep them open for the caller to manage.Changes
ProcessStartInfo.cs: NewStandardInput,StandardOutput,StandardErrorauto-properties (SafeFileHandle?) with XML docs. Remarks document that handles do not need to be inheritable (the runtime handles that), and list typical APIs (SafeFileHandle.CreateAnonymousPipe,File.OpenHandle,File.OpenNullHandle,Console.OpenStandardInputHandle/OpenStandardOutputHandle/OpenStandardErrorHandle), interaction withLeaveHandlesOpen, and constraints. NewLeaveHandlesOpenproperty (bool, defaultfalse) controlling whether handles are disposed after process start.ref/System.Diagnostics.Process.cs: Public API surface additions forStandardInput,StandardOutput,StandardError, andLeaveHandlesOpenProcess.csvalidation: When anyStandardXhandle is set,UseShellExecutemust befalseand the correspondingRedirectStandardXmust also befalse. Reuses existingSR.CantRedirectStreamserror message for theUseShellExecutecheck.Process.cspipe creation: New handles take priority over default stdin/stdout/stderr inheritance. Handles are always disposed after starting the child process unlessLeaveHandlesOpenistrue.Strings.resx: New error stringCantSetHandleAndRedirectProcessHandlesTests.cs: Tests for pipe redirection, inherited handles, piping, and validation error cases💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.