Add CreateNewProcessGroup to ProcessStartInfo #117105

jozkee · 2025-06-27T21:37:24Z

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

Copilot

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

Copilot · 2025-07-01T19:29:25Z

src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs

+
+                    Console.WriteLine(PosixSignalRegistrationCreatedMessage);
+
+                    while (!receivedSignal) ;


The busy-wait loop spins the CPU continuously. Consider using a synchronization primitive (e.g. ManualResetEventSlim) or at least inserting Thread.Sleep(1) (or Thread.Yield()) inside the loop to reduce CPU usage.

Suggested change

while (!receivedSignal) ;

while (!receivedSignal)

{

Thread.Sleep(1);

}

src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs

stephentoub · 2025-07-01T20:59:49Z

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartInfo.Unix.cs

@@ -52,5 +52,12 @@ public SecureString? Password
            get { throw new PlatformNotSupportedException(SR.Format(SR.ProcessStartSingleFeatureNotSupported, nameof(Password))); }
            set { throw new PlatformNotSupportedException(SR.Format(SR.ProcessStartSingleFeatureNotSupported, nameof(Password))); }
        }
+
+        [SupportedOSPlatform("windows")]


Are we throwing PNSE now on Unix as part of reserving the possibility of implementing this in the future? Or we think this is permanently Windows only?

It is possible to implement this in Unix, however, no one has shown any needs for it, so the throw is a good safeguard.

This API would not map well to the Unix concept of process group. I do not expect this API to be implemented on Unix ever.

stephentoub · 2025-07-01T21:00:17Z

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartInfo.Unix.cs

+        [SupportedOSPlatform("windows")]
+        public bool CreateNewProcessGroup
+        {
+            get { throw new PlatformNotSupportedException(SR.Format(SR.ProcessStartSingleFeatureNotSupported, nameof(CreateNewProcessGroup))); }


get doesn't need to throw, right? It could just always return false?

I was following the existing pattern but returning false does feel better.

returning false does feel better.

To me, it feels better to be consistent with the established pattern.

I'm not sure why we have that as a pattern in this particular class. Elsewhere we have the getters that just return the value do so even when the setters throw PNSE:

runtime/src/libraries/System.Console/src/System/ConsolePal.Unix.cs

Lines 297 to 329 in b509ab1

public static int BufferWidth

{

get { return WindowWidth; }

set { throw new PlatformNotSupportedException(); }

}

public static int BufferHeight

{

get { return WindowHeight; }

set { throw new PlatformNotSupportedException(); }

}

public static int LargestWindowWidth

{

get { return WindowWidth; }

}

public static int LargestWindowHeight

{

get { return WindowHeight; }

}

public static int WindowLeft

{

get { return 0; }

set { throw new PlatformNotSupportedException(); }

}

public static int WindowTop

{

get { return 0; }

set { throw new PlatformNotSupportedException(); }

}

Even elsewhere in Process we'll make getters work even when we throw PNSE from setters, e.g.

runtime/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessThread.Linux.cs

Lines 17 to 30 in b509ab1

private ThreadPriorityLevel PriorityLevelCore

{

// This mapping is relatively arbitrary. 0 is normal based on the man page,

// and the other values above and below are simply distributed evenly.

get

{

Interop.procfs.ParsedStat stat = GetStat();

return Interop.Sys.GetThreadPriorityFromNiceValue((int)stat.nice);

}

set

{

throw new PlatformNotSupportedException(); // We can find no API to set this on Linux

}

}

If we can minimize friction by having getters work, I think we should.

ProcessStartInfo.CreateNewProcessGroup and other similar ProcessStartInfo properties are typically only going be set. They are unlikely to be read by the app. If they are read by the app, we probably want the platform analyzer flag them since it is not expected to use these properties in platform neutral code. Throwing for both getter and setter makes the story coherent.

Console.BufferWidth/Height are likely to be read by platform-neutral code. It makes sense that we made the getter work where possible.

There are other niche properties on Console, like Console.TreatControlCAsInput, where both getter and setter throws on some platforms.

I think the pattern that I see is that we make the getter return dummy value instead of throwing only when there is a plausible scenario for it.

These wouldn't be dummy values. They're the default values the properties have, regardless of platform. For a property that defaults to false or null or 0 (rather than it needing to read some system state) I do not see what it helps having the getter throw PNSE. What is the benefit?

I do not think throwing vs. returning default value makes a difference for user experience for niche properties like this one. I think it just about our design guideline for the x-plat behavior of these properties. We want to be consistent. I believe that our preference is to throw currently.

If we decide that our preference is to return default value whenever possible instead, I do not have a problem with it but I do not see the value. If we do that, we may want to go over the property getters that are throwing PlatformNotSupportedException currently and change them to return default value where possible instead for consistency (and update their platform support annotations to match).

stephentoub · 2025-07-01T21:08:24Z

src/libraries/System.Diagnostics.Process/tests/ProcessTests.Windows.cs

+        // See https://learn.microsoft.com/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw#remarks:
+        // When a process is created with CREATE_NEW_PROCESS_GROUP specified, an implicit call to SetConsoleCtrlHandler(NULL,TRUE) 
+        // is made on behalf of the new process; this means that the new process has CTRL+C disabled.
+        private static unsafe void ReEnableCtrlCHandlerIfNeeded(PosixSignal signal)


If the purpose of this additional feature is to be able to send ctrl-C to a process, and if ctrl-C ends up being disabled by default in that process, isn't that going to be a problem?

It is a pit of failure in Windows, IMO, especially for apps that don't terminate with Ctrl+Break, like ping.exe.

The workaround is to use CreateProcess with a helper app that re-enables Ctrl+C and calls CreateProcess to the app you want to originally start, or if you have control over your target app, reenable Ctrl+C directly there.

Right, the parent and child processes have to cooperate to make the scheme work on WIndows. It is one of the differences between Windows and Unix designs for process groups.

Add CreateNewProcessGroup

50ed533

jozkee added this to the 10.0.0 milestone Jun 27, 2025

jozkee requested review from ericstj and a team June 27, 2025 21:37

jozkee self-assigned this Jun 27, 2025

jozkee added the area-System.Diagnostics.Process label Jun 27, 2025

Add RemoteExecutor.IsSupported condition

0ae9e3c

jozkee marked this pull request as ready for review July 1, 2025 19:28

Copilot AI review requested due to automatic review settings July 1, 2025 19:28

Copilot AI reviewed Jul 1, 2025

View reviewed changes

stephentoub reviewed Jul 1, 2025

View reviewed changes


		Console.WriteLine(PosixSignalRegistrationCreatedMessage);

		while (!receivedSignal) ;

	public static int BufferWidth
	{
	get { return WindowWidth; }
	set { throw new PlatformNotSupportedException(); }
	}

	public static int BufferHeight
	{
	get { return WindowHeight; }
	set { throw new PlatformNotSupportedException(); }
	}

	public static int LargestWindowWidth
	{
	get { return WindowWidth; }
	}

	public static int LargestWindowHeight
	{
	get { return WindowHeight; }
	}

	public static int WindowLeft
	{
	get { return 0; }
	set { throw new PlatformNotSupportedException(); }
	}

	public static int WindowTop
	{
	get { return 0; }
	set { throw new PlatformNotSupportedException(); }
	}

	private ThreadPriorityLevel PriorityLevelCore
	{
	// This mapping is relatively arbitrary. 0 is normal based on the man page,
	// and the other values above and below are simply distributed evenly.
	get
	{
	Interop.procfs.ParsedStat stat = GetStat();
	return Interop.Sys.GetThreadPriorityFromNiceValue((int)stat.nice);
	}
	set
	{
	throw new PlatformNotSupportedException(); // We can find no API to set this on Linux
	}
	}

Add CreateNewProcessGroup to ProcessStartInfo #117105

Are you sure you want to change the base?

Add CreateNewProcessGroup to ProcessStartInfo #117105

Uh oh!

Conversation

jozkee commented Jun 27, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkotas Jul 1, 2025 •

edited

Loading

stephentoub Jul 2, 2025 •

edited

Loading

jkotas Jul 2, 2025 •

edited

Loading

jkotas Jul 2, 2025 •

edited

Loading