Enable StartSuspended for all Unix platforms#130347
Enable StartSuspended for all Unix platforms#130347adamsitnik with Copilot wants to merge 4 commits into
Conversation
- Remove [SupportedOSPlatform] attributes from StartSuspended and Resume() - Remove platform check in SafeProcessHandle.Resume() and ProcessStartInfo validation - In native pal_process.c: remove ENOTSUP check, use fork() instead of vfork() when startSuspended is true, and raise(SIGSTOP) before execve - Close the exec-waiting pipe before SIGSTOP so parent can proceed - Update tests: remove PlatformSpecific attribute and non-supported test class Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
|
Tagging subscribers to this area: @dotnet/area-system-diagnostics-process |
There was a problem hiding this comment.
Pull request overview
This PR expands ProcessStartInfo.StartSuspended and SafeProcessHandle.Resume() beyond macOS by implementing “start suspended” on non-macOS Unix using a pre-execve self-stop (SIGSTOP) and resuming via SIGCONT, and updates the public contract/tests accordingly.
Changes:
- Updates native Unix process creation to support
startSuspendedin the fork/exec path (including fork-over-vfork selection). - Removes platform gating/attributes so
StartSuspended/Resume()are no longer limited to Windows+macOS. - Broadens the existing StartSuspended test coverage beyond Windows/macOS and removes now-obsolete “throws PNSE” tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/libs/System.Native/pal_process.c | Implements non-macOS Unix StartSuspended via SIGSTOP before execve and switches to fork() when suspended. |
| src/libraries/System.Diagnostics.Process/tests/StartSuspendedTests.cs | Removes Windows/macOS-only restriction and deletes tests expecting PNSE on other Unix OSes. |
| src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartInfo.cs | Updates docs and removes runtime platform validation for StartSuspended. |
| src/libraries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.cs | Removes platform gate/PNSE from Resume() and updates remarks to “Unix”. |
| src/libraries/System.Diagnostics.Process/ref/System.Diagnostics.Process.cs | Updates ref contract by removing Windows/macOS-only platform annotations for Resume() and StartSuspended. |
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot please address the feedback
…, immediate-resume test - pal_process.c: add waitpid(WUNTRACED) in parent after pipe close for startSuspended to eliminate SIGCONT/SIGSTOP race condition - ProcessStartInfo.cs: clarify in docs that on non-macOS Unix suspension occurs before execve (process image not yet replaced) - StartSuspendedTests.cs: add StartSuspended_ResumeImmediately_Succeeds test that starts suspended and immediately calls Resume Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
- Remove redundant processId > 0 guard (success implies positive PID) - Separate WIFEXITED/WIFSIGNALED cases with clearer ECHILD error and comment Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
adamsitnik
left a comment
There was a problem hiding this comment.
LGTM, @copilot please apply my suggestion
…ucceeds test Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
|
@adamsitnik does this suspend implementation make sense? Are the semantics sufficiently similar to Windows? You are between fork and exec, so what is suspended is not the actual process. |
My goal is to make it as close as possible to
We suspend the process via SIGSTOP before any user code executes. The provided PID is also valid. SIGCONT is used to resume. @tmds do you see any scenarios where it would fail? |
|
The implementation looks mechanically correct, but I think what it ends up doing may not enable the same things as it does on Windows and macOS. For example, if you'd attach a debugger on this suspended process, the target is not the application you want to debug. You can't set breakpoints, etc. So my question is whether this enables what the users of the API are likely expecting. |
This one suspends after the exec, which is a major difference with what this implementation does. To implement this on Linux, it seems we can use |
I agree.
But this would have some side effects that could also be not expected. I think I am going to close this PR as of now and simply wait for more user feedback. Perhaps people would be happy with what I am suggesting here or |
|
Why not use posix_spawn on Linux too? |
As per google AI: |
Extends
ProcessStartInfo.StartSuspendedandSafeProcessHandle.Resume()to work on all Unix platforms, not just macOS. Previously these APIs threwPlatformNotSupportedExceptionon Linux and other Unix systems.Approved API: #94127 (comment)
Implementation approach:
raise(SIGSTOP)beforeexecve, suspending itself untilResume()sendsSIGCONTfork()is used instead ofvfork()whenstartSuspendedis true (child must stop before exec, incompatible with vfork's shared-memory semantics)Changes:
[SupportedOSPlatform("windows")]/[SupportedOSPlatform("macos")]fromResume()andStartSuspendedSafeProcessHandle.cs: Removed platform gate andPlatformNotSupportedExceptionfromResume()ProcessStartInfo.cs: Removed[SupportedOSPlatform]attributes and platform validation checkpal_process.c: RemovedENOTSUPearly-return; added fork-over-vfork selection when suspended; added pipe-close +raise(SIGSTOP)beforeexecve[PlatformSpecific]restriction and theStartSuspendedTests_NonWindowsNonMacOSclass that tested the now-removed exception