Add Process.Start callback overload with ref struct ProcessStartArguments#128862
Add Process.Start callback overload with ref struct ProcessStartArguments#128862Copilot wants to merge 17 commits into
Conversation
Introduces a new Process.Start(ProcessStartInfo, Func<ProcessStartArguments, SafeProcessHandle>) overload that allows users to create processes using their own system calls while leveraging .NET's Process infrastructure (pipe management, async I/O, WaitForExit). - Added ProcessStartArguments class with platform-specific pointer properties - Added Windows implementation using BuildCommandLine/GetEnvironmentVariablesBlock - Added Unix implementation using ResolvePath/ParseArgv/AllocArgvArray/AllocEnvpArray - Updated ref assembly with new public API surface - Added Windows test using CreateProcess inside the callback - Added Unix test using posix_spawn inside the callback Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
|
Tagging subscribers to this area: @dotnet/area-system-diagnostics-process |
- reduce code duplication - acquire the locks! - fix Windows tests: enable inheritance - fix Unix implementation: handle "usesTerminal"
638b886 to
3163021
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a new public Process.Start overload that prepares stdio handles + argument/environment buffers and then delegates the actual OS process creation to a user callback, returning a Process wired up to existing Process infrastructure (streams, WaitForExit, etc.).
Changes:
- Adds new public API surface:
Process.Start(ProcessStartInfo, Func<ProcessStartArguments, SafeProcessHandle>)andProcessStartArguments. - Extends Windows/Unix start paths and
SafeProcessHandleto support invoking a callback-based start flow. - Adds Windows + Unix tests exercising callback-based process creation and output redirection.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Diagnostics.Process/tests/ProcessHandlesTests.Windows.cs | Adds a Windows test using CreateProcess inside the callback to validate stdout redirection. |
| src/libraries/System.Diagnostics.Process/tests/ProcessHandlesTests.Unix.cs | Adds a Unix test using posix_spawn inside the callback to validate stdout redirection. |
| src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartArguments.cs | Introduces the new argument carrier type passed to the callback. |
| src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs | Threads optional callback through the Windows StartCore flow. |
| src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs | Threads optional callback through the Unix StartCore flow and updates wait-state ownership logic. |
| src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs | Adds the new public Process.Start callback overload and refactors internal start flow. |
| src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj | Adds the new ProcessStartArguments.cs to compilation. |
| src/libraries/System.Diagnostics.Process/src/Resources/Strings.resx | Adds a resource string for invalid callback handle errors. |
| src/libraries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs | Adds Windows callback start helper that builds/pins command line + env block and enables handle inheritance. |
| src/libraries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Unix.cs | Adds Unix callback start helper that allocates argv/envp and integrates wait-state registration. |
| src/libraries/System.Diagnostics.Process/ref/System.Diagnostics.Process.csproj | Enables unsafe blocks for the ref assembly build due to pointer-typed API. |
| src/libraries/System.Diagnostics.Process/ref/System.Diagnostics.Process.cs | Updates ref surface with new public Process.Start overload and ProcessStartArguments. |
| src/libraries/Common/src/Interop/Unix/System.Native/Interop.ForkAndExecProcess.cs | Promotes argv/envp allocation helpers to internal for reuse. |
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot please address my feedback
…local const, redirect stderr Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot address my feedback
| byte[] pathBytes = System.Text.Encoding.UTF8.GetBytes(args.FileName! + '\0'); | ||
| fixed (byte* pathPtr = pathBytes) |
There was a problem hiding this comment.
The users don't need to perform any kind of encoding/decoding for other parameters like args or env vars, they should not need to do it for FileName as well.
@copilot Since FileName is used only on Unix, we should do the following:
- rename
FileNametoResolvedPath - change the type of the property from
string?tobyte* - annotate the property with
[UnsupportedOSPlatform("windows")] - update
SafeProcessHandle.Unix.csto provide us with pinnedbyte*, also ensure the encoding is done without allocating any managed memory
There was a problem hiding this comment.
Addressed in 3079bdd. ProcessStartArguments.FileName is now ResolvedPath (byte*, [UnsupportedOSPlatform("windows")]), Unix start now provides the UTF-8 null-terminated pointer without managed byte[] allocation, and the Unix callback test now uses args.ResolvedPath directly.
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
| public bool StandardError { get { throw null; } } | ||
| public override string ToString() { throw null; } | ||
| } | ||
| public sealed partial class ProcessStartArguments |
There was a problem hiding this comment.
Something to consider: Should this be ref struct to limit the lifetime to the callback by construction?
There was a problem hiding this comment.
Sounds very reasonable. But this would force us to introduce a dedicated delegate type?
There was a problem hiding this comment.
Yes - if we wanted to pass the structs as in.
There was a problem hiding this comment.
@copilot please give this idea a try (make it ref struct, but don't introduce a dedicated delegate if you don't have to), but separate it from other changes (use dedicated commit)
There was a problem hiding this comment.
Done in d30a6f62: ProcessStartArguments is now a ref struct, and I kept this as a dedicated commit separate from other changes.
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot please address my feedback
| public bool StandardError { get { throw null; } } | ||
| public override string ToString() { throw null; } | ||
| } | ||
| public sealed partial class ProcessStartArguments |
There was a problem hiding this comment.
@copilot please give this idea a try (make it ref struct, but don't introduce a dedicated delegate if you don't have to), but separate it from other changes (use dedicated commit)
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Addressed in two commits: |
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
main PR
Description
Adds
Process.Start(ProcessStartInfo, Func<ProcessStartArguments, SafeProcessHandle>)to let callers create processes with custom platform APIs while still usingProcessinfrastructure (ReadAllText, async stream handling,WaitForExit, etc.).This PR also incorporates review feedback across the callback path and tests, including:
cmd /c echo ... 1>&2output formatting ("error \r\n"with trailing space before\r\n)Most notably, callback executable-path data and callback argument lifetime behavior were updated based on feedback:
ProcessStartArguments.FileNamewas replaced with Unix-onlyProcessStartArguments.ResolvedPathResolvedPathis nowbyte*and annotated with[UnsupportedOSPlatform("windows")]byte[]allocationargs.ResolvedPathdirectly forposix_spawnProcessStartArgumentsis now aref structto constrain usage to callback lifetime by constructionAdditional Unix callback-path update from feedback:
2inDEBUG,256otherwise) to exercise both allocation paths in debug test runsstackallocbuffer and slices to the required lengthCustomer Impact
Developers who need unsupported/native process creation mechanisms (for example custom
CreateProcess*/posix_spawnflows) can still rely on .NETProcessfeatures instead of reimplementing process I/O and lifetime handling themselves.Regression
No known product regression is being fixed. This is a new callback-based API path plus follow-up correctness and API-shape refinements from review feedback.
Testing
Validated with targeted builds in this repo:
./dotnet.sh build src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj -c Release -v minimal /p:BuildProjectReferences=false./dotnet.sh build src/libraries/System.Diagnostics.Process/ref/System.Diagnostics.Process.csproj -c Release -v minimal /p:BuildProjectReferences=falseSystem.Diagnostics.Process.Testsfull project build remains blocked in this environment by existing shared-framework prerequisite errors (eng/targetingpacks.targets), unrelated to these code changes.Risk
Moderate. This change adds new public API surface and adjusts callback argument shape on Unix (
ResolvedPathpointer) and callback-lifetime constraints (ProcessStartArgumentsasref struct).Risk is mitigated by:
Package authoring no longer needed in .NET 9
IMPORTANT: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older versions.