Implement SafeProcessHandle APIs for macOS#124923
Implement SafeProcessHandle APIs for macOS#124923Copilot wants to merge 25 commits intocopilot/add-new-apis-to-safeprocesshandlefrom
Conversation
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
…rocess path) Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot please address my feedback.
Moreover, please modify the .yml file that is required to let the CI run for this branch. I am not an expert in this area, but I think it should be enough to modify this file:
runtime/eng/pipelines/runtime.yml
Lines 33 to 37 in fdaebbf
and list copilot/*.* after release/*.*
- release/*.*
- copilot/*.*
src/libraries/Common/src/Interop/Unix/System.Native/Interop.SpawnProcess.cs
Outdated
Show resolved
Hide resolved
...braries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/SafeProcessHandleTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/SafeProcessHandleTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/SafeProcessHandleTests.cs
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/SafeProcessHandleTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/SafeProcessHandleTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/SafeProcessHandleTests.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/SafeProcessHandleTests.Unix.cs
Show resolved
Hide resolved
…on throws, update tests, add CI branch trigger Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
All feedback addressed and |
There was a problem hiding this comment.
Pull request overview
This PR implements SafeProcessHandle APIs for macOS using posix_spawn for process creation and kqueue for wait/timeout/cancellation operations. The implementation provides parity with the Windows implementation, enabling cross-platform process management through the new SafeProcessHandle APIs.
Changes:
- Native layer: Added macOS-specific implementations using
posix_spawn,kqueue, andwaitpidfor process creation and management - Managed layer: Implemented all core SafeProcessHandle methods (
Start,Open,WaitForExit*,Kill,Resume,Signal) with proper error handling - Code refactoring: Extracted shared utility methods (
CreateEnvp,AllocNullTerminatedArray,FreeArray) fromInterop.ForkAndExecProcess.cstoProcessUtils.Unix.csfor reuse - Tests: Extended cross-platform tests to support macOS and added macOS-specific signal tests
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/libs/configure.cmake | Added CMake feature detection for posix_spawn APIs and kqueue on macOS |
| src/native/libs/Common/pal_config.h.in | Added configuration defines for detected posix_spawn features |
| src/native/libs/System.Native/pal_process.h | Declared new native API functions for process spawning, signaling, and waiting |
| src/native/libs/System.Native/pal_process.c | Implemented macOS-specific process APIs using posix_spawn and kqueue, including signal mapping |
| src/native/libs/System.Native/pal_process_wasi.c | Added ENOTSUP stubs for WASI platform |
| src/native/libs/System.Native/entrypoints.c | Registered new native function entry points |
| src/libraries/Common/src/Interop/Unix/System.Native/Interop.SpawnProcess.cs | Added P/Invoke declarations for new native APIs |
| src/libraries/Common/src/Interop/Unix/System.Native/Interop.ForkAndExecProcess.cs | Refactored to use shared ProcessUtils methods |
| src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessUtils.Unix.cs | Added shared utility methods for environment and array marshalling |
| src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs | Updated to use ProcessUtils.CreateEnvp instead of local implementation |
| src/libraries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Unix.cs | Implemented all SafeProcessHandle core methods for macOS |
| src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj | Added Interop.SpawnProcess.cs and Interop.Pipe.cs to Unix builds |
| src/libraries/System.Diagnostics.Process/tests/SafeProcessHandleTests.cs | Extended platform support to include macOS and updated tests for cross-platform compatibility |
| src/libraries/System.Diagnostics.Process/tests/SafeProcessHandleTests.Unix.cs | Added macOS-specific signal tests |
| src/libraries/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj | Added SafeProcessHandleTests.Unix.cs to Unix test builds |
| eng/pipelines/runtime.yml | Added copilot/. branch trigger (intended for development, should be removed) |
src/libraries/System.Diagnostics.Process/tests/SafeProcessHandleTests.cs
Outdated
Show resolved
Hide resolved
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot please address all unresolved feedback
…ping, fix pwd test Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
All unresolved feedback addressed in cdd13b0: simplified |
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot please give my suggestion a try.
…hecks Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot please address my feedback
Also, annotate the Open_CurrentProcess_Succeeds test with [PlatformSpecific(TestPlatforms.Windows)]
| throw new Win32Exception(); | ||
| } | ||
|
|
||
| return new SafeProcessHandle(pidfd, processId, isGroupLeader: false); |
There was a problem hiding this comment.
Should this set isGroupLeader based on whether the process is group leader or not?
I am not following why we need to have cached _isGroupLeader flag on SafeProcessHandle in the first place.
There was a problem hiding this comment.
I am not following why we need to have cached
_isGroupLeaderflag on SafeProcessHandle in the first place.
To ensure the WaitForExitOrKillOn* methods kill entire process group rather than just the leader.
There was a problem hiding this comment.
Do you need to cache it on SafeProcesssHandle for that? You can always check whether the process is a process group leader by calling the system APIs, especially if it is only needed on a rare expensive path.
There was a problem hiding this comment.
Do you need to cache it on SafeProcesssHandle for that? You can always check whether the process is a process group leader by calling the system APIs, especially if it is only needed on a rare expensive path.
Do you have any data to prove that this is going to be a rare execution path? Or any data showing that our customers have plenty of SafeProcesssHandle instances in their apps and the size of SafeProcesssHandle is important enough to not cache such information?
There was a problem hiding this comment.
My primary concern is correctness: The cached isGroupLeader is not going to match the actual state when the handle is created through this path.
Performance is not my primary concern.
There was a problem hiding this comment.
Fixed the correctness issue in 33db32f. SystemNative_OpenProcess now checks getpgid(pid) == pid and returns the result via a new out_isGroupLeader parameter. OpenCore uses this to set _isGroupLeader accurately instead of hardcoding false.
| } | ||
|
|
||
| private SafeProcessHandle(int pidfd, int pid, bool isGroupLeader) | ||
| : this(existingHandle: (IntPtr)pidfd, ownsHandle: true) |
There was a problem hiding this comment.
SafeProcessHandle.DangerousGetHandle returns waithandle today. Does this change mean that it can return pidfd in some cases? I think it would be a breaking change.
…esshandle' into copilot/implement-safeprocesshandle-apis
… codes on Unix can't be > 255, so we can't return PID as exit code
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot Please address my feedback
| } | ||
|
|
||
| // Call native library to spawn process | ||
| int result = Interop.Sys.SpawnProcess( |
There was a problem hiding this comment.
@copilot please address this feedback
First of all, we can't use SafeFileHandle.Type to check whether provided standard handles are terminal handles because for both Terminal handles and /dev/null the API returns FileHandleType.CharacterDevice.
But luckily for us, there is already an API for checking that:
Please extend Interop.IsATty.cs with an overload that takes SafeFileHandle:
[LibraryImport(Libraries.SystemNative, EntryPoint = "SystemNative_IsATty")]
[return: MarshalAs(UnmanagedType.Bool)]
internal static partial bool IsATty(SafeFileHandle fd);And use it in StartCore (part of SafeProcessHandle.Unix.cs) to detect if any of the inputHandle/outputHandle/errorHandle point to a terminal:
bool usesTerminal = Interop.Sys.IsATty(inputHandle) || Interop.Sys.IsATty(outputHandle) || Interop.Sys.IsATty(errorHandle);Moreover, please add a comment explaining why we are doing this similar to this one.
Then move all the logic from src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.ConfigureTerminalForChildProcesses.Unix.cs to src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessUtils.ConfigureTerminalForChildProcesses.Unix.cs, and all the logic from src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.ConfigureTerminalForChildProcesses.iOS.cs to src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessUtils.ConfigureTerminalForChildProcesses.iOS.cs. Modify the src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj file to reflect the changes.
Then update all the calls from Process.Unix.cs and other files like ProcessWaitState.Unix.cs to the moved APIs to call the methods via ProcessUtils type.
Then update StartCore (part of SafeProcessHandle.Unix.cs) to call ProcessUtils.ConfigureTerminalForChildProcesses(1) before starting the process when usesTerminal is true and ProcessUtils.ConfigureTerminalForChildProcesses(-1) in case the Interop.Sys.SpawnProcess failed to spawn the process (and usesTerminal is true).
Extend StartCore to store usesTerminal in a readonly field (_usesTerminal) of SafeProcessHandle.Unix.cs.
Then modify all the WaitForExit* methods (and TryWaitForExitCore) in the src/libraries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Unix.cs file to call ProcessUtils.ConfigureTerminalForChildProcesses(-1) when _usesTerminal is true and the process has exited. Ideally move this logic to small helper method (OnProcessExited) and just call the helper everywhere.
…e via getpgid Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot I've asked you to address #124923 (comment) and you seem to have ignored it. Do address this particular feedback.
src/libraries/Common/src/Interop/Unix/System.Native/Interop.SpawnProcess.cs
Outdated
Show resolved
Hide resolved
…d is already used Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Applied the parameter rename in af33fd5. |
|
|
||
| private static SafeProcessHandle OpenCore(int processId) | ||
| { | ||
| int result = Interop.Sys.OpenProcess(processId, out int pidfd, out int isGroupLeader); |
There was a problem hiding this comment.
I believe that processes can change process groups as long as they stay within the same session. Is that correct? It means that this cached value can be stale.
… to ProcessUtils - Add IsATty(SafeFileHandle) overload to Interop.IsATty.cs - Move ConfigureTerminalForChildProcesses logic from Process.*.cs to ProcessUtils.*.cs - Update Process.*.cs to delegate to ProcessUtils - Add _usesTerminal field to SafeProcessHandle.Unix.cs - Detect terminal usage in StartCore with IsATty - Add OnProcessExited helper, called from all WaitForExit* methods - Update csproj with new files and IsATty reference Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot address my feedback
...iagnostics.Process/src/System/Diagnostics/Process.ConfigureTerminalForChildProcesses.Unix.cs
Outdated
Show resolved
Hide resolved
…te all callers to use ProcessUtils directly Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Done in e88f121 — removed both |
Description
Implements the macOS-specific managed and native layers for the new
SafeProcessHandleAPIs (Start,Open,WaitForExit*,Kill,Resume,Signal), usingposix_spawnfor process creation andkqueuefor wait/timeout/cancellation.Managed changes
SafeProcessHandle.Unix.cs: Full implementation ofStartCore,OpenCore,WaitForExitCore,TryWaitForExitCore,WaitForExitOrKillOnTimeoutCore,WaitForExitAsyncCore,WaitForExitOrKillOnCancellationAsyncCore,KillCore,ResumeCore,SendSignalCore. UsesNoPidFd = int.MinValue(not-1) since the handle derives fromSafeHandleZeroOrMinusOneIsInvalid. All error paths usethrow new Win32Exception()(parameterless) to let it fetch the last error automatically.OpenCorecorrectly detects group leader status by callinggetpgid(pid)via nativeOpenProcessand comparing withpid, rather than hardcodingfalse.StartCoredetects terminal usage viaInterop.Sys.IsATtyon stdin/stdout/stderr and callsProcessUtils.ConfigureTerminalForChildProcessesaccordingly, with propers_processStartLockread/write lock usage._usesTerminalreadonly field is stored and used byOnProcessExitedhelper (called from allWaitForExit*methods) to decrement terminal child count on exit.ProcessUtils.Unix.cs: ExtractedCreateEnvp,AllocNullTerminatedArray,FreeArrayasinternalshared methods, reused by bothProcess.Unix.csandSafeProcessHandle.Unix.cs.ProcessUtils.ConfigureTerminalForChildProcesses.Unix.cs(new): Moved terminal configuration logic (ConfigureTerminalForChildProcesses,SetDelayedSigChildConsoleConfigurationHandler,DelayedSigChildConsoleConfiguration,AreChildrenUsingTerminal) fromProcesstoProcessUtilsfor shared access by bothProcessandSafeProcessHandle.ProcessUtils.ConfigureTerminalForChildProcesses.iOS.cs(new): iOS stub with[Conditional("unnecessary")]no-ops, mirroring the originalProcess.ConfigureTerminalForChildProcesses.iOS.cs.Process.Unix.cs: All callers updated to callProcessUtils.ConfigureTerminalForChildProcesses,ProcessUtils.SetDelayedSigChildConsoleConfigurationHandler, andProcessUtils.AreChildrenUsingTerminaldirectly (no wrapper methods).ProcessWaitState.Unix.cs: Updated to callProcessUtils.ConfigureTerminalForChildProcessesdirectly instead of throughProcess.Process.ConfigureTerminalForChildProcesses.Unix.cs: Removed — all callers now useProcessUtilsdirectly.Process.ConfigureTerminalForChildProcesses.iOS.cs: Removed — all callers now useProcessUtilsdirectly.Interop.IsATty.cs: AddedSafeFileHandleoverload for terminal detection inStartCore.Interop.SpawnProcess.cs: NewLibraryImportdeclarations (EntryPoint = "SystemNative_*") forSpawnProcess,SendSignal,WaitForExitAndReap,TryWaitForExit,TryWaitForExitCancellable,WaitForExitOrKillOnTimeout,TryGetExitCode,OpenProcess.OpenProcessreturns the group leader status via anisGroupLeaderparameter.Interop.ForkAndExecProcess.cs: Delegates to sharedProcessUtilsmethods.Interop.Pipe.cs,Interop.SpawnProcess.cs,Interop.IsATty.cs, and newProcessUtils.ConfigureTerminalForChildProcesses.*.csfiles for Unix builds. Removed references to deletedProcess.ConfigureTerminalForChildProcesses.*.csfiles.Native changes
pal_process.c: macOS implementation usingposix_spawnwithPOSIX_SPAWN_CLOEXEC_DEFAULT,posix_spawn_file_actions_addinherit_np,posix_spawn_file_actions_addchdir_np,POSIX_SPAWN_START_SUSPENDED. The implementation is guarded by#if defined(__APPLE__) && !defined(TARGET_MACCATALYST) && !defined(TARGET_TVOS)to exclude Mac Catalyst and tvOS (iOS is supported).posix_spawn_file_actions_addchdir_npretainsHAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCHDIR_NPfeature detection since it is not available on tvOS. Wait functions usekqueue/EVFILT_PROC/NOTE_EXIT(guarded by existingHAVE_KQUEUE) for timeout and cancellation support. Signal mapping reusesSystemNative_GetPlatformSignalNumberandTryConvertSignalCodeToPosixSignalfrompal_signal.cinstead of duplicating the logic. Environment variable resolution is extracted into aGetEnvVarshelper for reuse.OpenProcessuseskill(pid, 0)to check process existence without requiring a child-process relationship, and detects group leader status viagetpgid(pid) == pid, returning the result through anout_isGroupLeaderoutput parameter. BothSpawnProcessandOpenProcesssetout_pidfdtoINT_MIN(not-1) since the managed layer treats-1as an invalid handle due toSafeHandleZeroOrMinusOneIsInvalid.pal_signal.c/pal_signal.h: MadeTryConvertSignalCodeToPosixSignalnon-static and added its declaration to the header for reuse bypal_process.c.pal_process_wasi.c: Stubs returning-1witherrno = ENOTSUP.pal_process.h: New function declarations with reorderedSpawnProcessarguments (path, argv, envp, working_dir, inherited_handles, inherited_handles_count, stdin/stdout/stderr fds, flags, out params).OpenProcesssignature includesout_isGroupLeaderparameter.entrypoints.c: NewDllImportEntryentries.configure.cmake/pal_config.h.in: AddedHAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCHDIR_NPfeature detection (not available on tvOS). No otherHAVE_POSIX_SPAWN*defines are added — macOS platform checks (#ifdef __APPLE__) are used instead. ExistingHAVE_KQUEUEis reused forsys/event.hinclusion.Test changes
SafeProcessHandleTests.cs: Extended[PlatformSpecific]toTestPlatforms.OSX | TestPlatforms.Windows. AllProcessStartOptionscreation uses platform detection (OperatingSystem.IsWindows()ternary).CreateTenSecondSleepreturnssleep 10on Unix.Start_WithNoArguments_Succeedsuseshostnameon Windows andpwdon Unix (platform-specific, sincepwdis not a standalone executable on Windows). Kill tests assertPosixSignal.SIGKILLand exit code > 128 on Unix.Open_CurrentProcess_Succeedsis annotated[PlatformSpecific(TestPlatforms.Windows)].SafeProcessHandleTests.Unix.cs: macOS-specific tests forSignal(SIGTERM),Signal(SIGINT), invalid signal, and signal-to-exited-process behavior. All long-running tests useCreateTenSecondSleep(). TheProcessIdtest usesstarted.Kill()(not the non-owning copy) since the copy handle has no valid pid.CI changes
eng/pipelines/runtime.yml: Addedcopilot/*.*to CI branch triggers to enable pipeline runs for this branch.💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.