Add macOS support for StartSuspended via POSIX_SPAWN_START_SUSPENDED#129570
Add macOS support for StartSuspended via POSIX_SPAWN_START_SUSPENDED#129570Copilot wants to merge 8 commits into
Conversation
…and Resume via SIGCONT 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
…plify ResumeCore, update WASI stub, fix tests Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
All feedback addressed in commit 3e2d0d6:
All 635 tests pass with 0 failures. |
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot address my feedback
…idate() before ResumeCore Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Both changes applied in the latest commit: reformatted the |
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot address my feedback and run the tests locally before pushing the changes
|
/azp list |
|
/azp run runtime-maccatalyst |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/libraries/System.Diagnostics.Process/tests/StartSuspendedTests.cs:66
- The test suite no longer validates
Resume()’s documented error behavior on macOS: the “Resume called twice” test was removed entirely, andResume_OnNonSuspendedProcess_ThrowsInvalidOperationExceptionis now Windows-only. Given the public XML docs forSafeProcessHandle.Resume()say it throws when the process wasn’t started suspended or has already been resumed, we should keep coverage for these cases on macOS once the implementation enforces the same contract there.
[ConditionalFact]
[PlatformSpecific(TestPlatforms.Windows)]
public void Resume_OnNonSuspendedProcess_ThrowsInvalidOperationException()
{
Process process = CreateProcess(static () => RemoteExecutor.SuccessExitCode);
using SafeProcessHandle processHandle = SafeProcessHandle.Start(process.StartInfo);
Assert.Throws<InvalidOperationException>(() => processHandle.Resume());
processHandle.WaitForExitOrKillOnTimeout(TimeSpan.FromMilliseconds(WaitInMS));
}
| if ((OperatingSystem.IsWindows() || OperatingSystem.IsMacOS()) && StartSuspended && UseShellExecute) | ||
| { | ||
| throw new InvalidOperationException(SR.StartSuspendedNotCompatible); | ||
| } |
There was a problem hiding this comment.
Applied in the latest commit. ProcessStartInfo.Validate() now throws PlatformNotSupportedException when StartSuspended=true on non-Windows/non-macOS platforms, before checking the UseShellExecute incompatibility.
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot please address my feedback
| if ((OperatingSystem.IsWindows() || OperatingSystem.IsMacOS()) && StartSuspended && UseShellExecute) | ||
| { | ||
| throw new InvalidOperationException(SR.StartSuspendedNotCompatible); | ||
| } |
…nded on unsupported OS, update Resume() docs and Windows impl, return error in fork() path Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
All three items addressed in the latest commit:
|
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot address my feedback
…ndle after resume Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Addressed in the latest commit — |
Extends
ProcessStartInfo.StartSuspendedandSafeProcessHandle.Resume()to macOS usingPOSIX_SPAWN_START_SUSPENDED. Approved API: #94127 (comment)Changes
[SupportedOSPlatform("macos")]toStartSuspendedandResume()in ref and srcInterop.Sys.ForkAndExecProcessmanaged wrapper now acceptsProcessStartInfodirectly (instead of individual bool parameters) and extractsStartDetached,KillOnParentExit, andStartSuspendedinternallypal_process.c): SetPOSIX_SPAWN_START_SUSPENDEDflag in the macOSposix_spawnpath whenstartSuspendedis true; returnsENOTSUPin thefork()path whenstartSuspendedis true to prevent silent behavior differences when credentials force the fork path on macOSpal_process_wasi.c): Updated WASI stub signature to includestartSuspendedparameterResumeCorecallsSignalCore(PosixSignal.SIGCONT); sharedResume()throwsPlatformNotSupportedExceptionon unsupported OS;Resume()no longer prohibits multiple calls — OS errors are surfaced directly asWin32ExceptionProcessStartInfo.Validate()throwsPlatformNotSupportedExceptionwhenStartSuspended=trueon non-Windows/non-macOS platforms; throwsInvalidOperationExceptionforUseShellExecuteincompatibility on supported platformsResume()docs to remove the "can only be called once" restriction and document per-platform behaviorStartSuspendedTestsfromTestPlatforms.WindowstoTestPlatforms.Windows | TestPlatforms.OSX;Resume_OnNonSuspendedProcesstest (Windows-only) now expectsWin32Exceptionfrom the OS instead ofInvalidOperationException; non-supported OS test verifiesPlatformNotSupportedExceptionUsage