dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.36k stars 4.74k forks source link

Add CreateNewProcessGroup to ProcessStartInfo #44944

Open mladedav opened 3 years ago

mladedav commented 3 years ago

Background and Motivation

Creating process groups is necessary to be able to send signals on Windows as they are sent to the whole group and the calling process may not know the group ID itself. POSIX systems have the same notion, however in unix-like systems one can use signals directly with processes, which is impossible in Windows as I understand it.

This has been previously discussed in another issue in 2017 but it turned out that this change itself would not help the requesting team so it was dropped at that time. For the record, I am using proposals from that time.

https://github.com/dotnet/runtime/issues/20734

Proposed API

namespace System.Diagnostics
{
     public sealed class ProcessStartInfo
     {
+          public bool CreateNewProcessGroup { get; set; }
     }
}

Usage Examples

var startInfo = new System.Diagnostics.ProcessStartInfo
{
    FileName = "powershell",
    Arguments = "./script-waiting-for-interrupt.ps1",
    CreateNewProcessGroup = true,
};
var p = new System.Diagnostics.Process.Start(startInfo);

Alternative Designs

Alternative discussed in the linked issue was using creation flags such as:

[Flags]
public enum CreationFlags
{
    CREATE_NEW_CONSOLE = 0x00000010,
    **CREATE_NEW_PROCESS_GROUP = 0x00000200**,
    CREATE_BREAKAWAY_FROM_JOB = 0x010000000,
    CREATE_DEFAULT_ERROR_MODE = 0x04000000,
    ...
};

But this turns somewhat Windows-centric which is not desired. Using a bool property instead is similar to how CreateNoWindow is exposed in ProcessStartInfo.

Risks

I do not have deep knowledge on this subject but I do not see any risks. I would assume the default behavior would be kept the same so no breaking changes to existing code. I do not know of any circumstances where creating a process in new group would prevent the process from creation (such as clashing with CreateNoWindow flag), but that is potential risk. Stopping applications properly may be more challenging for developers using this as the spawned processes may not get signals generated by user interaction with terminal.

ghost commented 3 years ago

Tagging subscribers to this area: @eiriktsarpalis, @jeffhandley See info in area-owners.md if you want to be subscribed.

Issue Details
## Background and Motivation Creating process groups is necessary to be able to send signals on Windows as they are sent to the whole group and the calling process may not know the group ID itself. POSIX systems have the same notion, however in unix-like systems one can use signals directly with processes, which is impossible in Windows as I understand it. This has been previously discussed in another issue in 2017 but it turned out that this change itself would not help the requesting team so it was dropped at that time. For the record, I am using proposals from that time. https://github.com/dotnet/runtime/issues/20734 ## Proposed API ```diff namespace System.Diagnostics { public sealed class ProcessStartInfo { + public bool CreateNewProcessGroup { get; set; } } } ``` ## Usage Examples ## Alternative Designs Alternative discussed in the linked issue was using creation flags such as: ``` C# [Flags] public enum CreationFlags { CREATE_NEW_CONSOLE = 0x00000010, **CREATE_NEW_PROCESS_GROUP = 0x00000200**, CREATE_BREAKAWAY_FROM_JOB = 0x010000000, CREATE_DEFAULT_ERROR_MODE = 0x04000000, ... }; ``` But this turns somewhat Windows-centric which is not desired. Using a bool property instead is similar to how `CreateNoWindow` is exposed in ProcessStartInfo. ## Risks I do not have deep knowledge on this subject but I do not see any risks. I would assume the default behavior would be kept the same so no breaking changes to existing code. I do not know of any circumstances where creating a process in new group would prevent the process from creation (such as clashing with CreateNoWindow flag), but that is potential risk. Stopping applications properly may be more challenging for developers using this as the spawned processes may not get signals generated by user interaction with terminal.
Author: mladedav
Assignees: -
Labels: `api-suggestion`, `area-System.Diagnostics.Process`, `untriaged`
Milestone: -
eiriktsarpalis commented 3 years ago

If I get this correctly, in order to obtain the same behaviour as CREATE_NEW_PROCESS_GROUP on Unix we would need to call setpgid(0, 0); between fork and execve?

mladedav commented 3 years ago

I believe so. That would put the new process into a new process group with ID identical to that new process' ID.

eiriktsarpalis commented 3 years ago

My concern about this proposal is that it seems to only allow a very constrained use case for process groups. What if I wanted to create a process group that includes the current process?

It is probably telling that in the original issue it was determined that ProcessStartInfo is probably not the appropriate API for making this type of configuration. So if we can't provide a general-purpose solution here, perhaps we shouldn't be adding it at all.

mladedav commented 3 years ago

As I understand the comment, ProcessStartInfo wasn't appropriate for the advanced usage PaulHigin needed, not for the flag itself. I can't think of a better place for this since this needs to be set before the process is started, but I do not mind it being anywhere else.

The constraints are based on what both Windows and Linux can do - Windows only has a flag CREATE_NEW_PROCESS_GROUP for CreaetProcess* functions while Linux is able to change the group ID dynamically after fork.

If you want the new process to be part of the creating process' group, you simply don't specify the option, it is the default in both OSs.

adamsitnik commented 3 years ago

Triage:

We should do research and find out if Windows allows for modifying process group after process creation as Unix does. If it does, we should consider adding APIs for all possibilities:

If it's not possible, the current proposal looks as good as it is.

Tyrrrz commented 3 years ago

Note: as @mladedav initially pointed out, the primary goal of this feature is to allow sending signals to the process on Windows. If Process somehow exposed this functionality directly (e.g. with a process.SendSignal(...) method), then this switch would not be needed and may become an implementation detail (i.e. Process.Start(...) may still create a new process group, just not expose it to the caller).

ItsRevolt commented 10 months ago

I am affected by this as well in a very similar way to the above mentioned issue (https://github.com/pulumi/pulumi-dotnet/issues/124)

Wanted to comment on this just to show there is a want for this, as the previous issue was closed due to lack of use by devs.

lennybacon commented 6 months ago

Any updates?