Skip to content

Conversation

jozkee
Copy link
Member

@jozkee jozkee commented Jun 27, 2025

Will fix #44944, provided that API review approves it.

@jozkee jozkee marked this pull request as ready for review July 1, 2025 19:28
@Copilot Copilot AI review requested due to automatic review settings July 1, 2025 19:28
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for creating a new process group via a CreateNewProcessGroup property on ProcessStartInfo. It extends the Windows implementation, stubs the API on Unix with proper exceptions, and includes interop constants plus cross-platform tests.

  • Introduces a new CreateNewProcessGroup property in ProcessStartInfo (Windows) and a stub on Unix.
  • Updates the Windows StartWithCreateProcess path to include the CREATE_NEW_PROCESS_GROUP flag and adds the corresponding interop constant.
  • Adds tests to verify the property behavior on both platforms and a signal‐handling scenario using remote executor.

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/libraries/System.Diagnostics.Process/tests/*.csproj Linked additional interop files: BOOL, SetConsoleCtrlHandler, Unix Libraries, and PosixSignal helpers
src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs Added TestCreateNewProcessGroup_HandlerReceivesExpectedSignal to exercise signal dispatch
src/libraries/System.Diagnostics.Process/tests/ProcessTests.Windows.cs Implemented SendSignal and ReEnableCtrlCHandlerIfNeeded for Windows
src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs Implemented SendSignal stub and no-op ReEnableCtrlCHandlerIfNeeded for Unix
src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs Added unit tests for CreateNewProcessGroup on Windows and Unix
src/libraries/System.Diagnostics.Process/tests/Interop.cs Added GenerateConsoleCtrlEvent DllImport
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartInfo.Windows.cs Added CreateNewProcessGroup property
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartInfo.Unix.cs Added stub CreateNewProcessGroup with PlatformNotSupportedException
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs ORs CREATE_NEW_PROCESS_GROUP into the creation flags
src/libraries/Common/src/Interop/Windows/Advapi32/Interop.ProcessOptions.cs Added CREATE_NEW_PROCESS_GROUP constant

@jozkee
Copy link
Member Author

jozkee commented Jul 7, 2025

RemoteExecutor timeouts on CI (1, 2), presumably GenerateConsoleCtrlEvent fails silently due to weridness on how tests are run. A somewhat related case is described in System.Console manual tests.
@adamsitnik, @ericstj, any thoughts?

I think preventing the test from running on CI but keep running locally may be good enough for Windows.

@jozkee
Copy link
Member Author

jozkee commented Jul 9, 2025

Error narrowed to only the Windows docker image, I suspect is because is running without -t (tty/console). I tested locally with docker and was able to observe a similar behavior.

https://helixr1107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-117105-merge-4803f8e0334a41609c/System.Diagnostics.Process.Tests/1/console.e6877ea7.log

@dotnet/runtime-infrastructure can you please confirm and take a look?

@jozkee jozkee requested a review from adamsitnik July 9, 2025 16:27
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The product changes looks good to me, I had some questions regarding the tests, but mostly to get a better understanding. Big thanks for taking the extra effort to implement the tests for the new feature! 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add CreateNewProcessGroup to ProcessStartInfo

5 participants