Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CreateNewProcessGroup to ProcessStartInfo #44944

Open
mladedav opened this issue Nov 19, 2020 · 9 comments
Open

Add CreateNewProcessGroup to ProcessStartInfo #44944

mladedav opened this issue Nov 19, 2020 · 9 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Diagnostics.Process needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Milestone

Comments

@mladedav
Copy link

mladedav commented Nov 19, 2020

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.

#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.

@mladedav mladedav added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 19, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Diagnostics.Process untriaged New issue has not been triaged by the area owner labels Nov 19, 2020
@ghost
Copy link

ghost commented Nov 19, 2020

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.

#20734

Proposed API

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:

[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
Copy link
Member

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
Copy link
Author

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

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Nov 23, 2020

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
Copy link
Author

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
Copy link
Member

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:

  • getting and setting process group of a given process instance
  • specify it during creating

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

@adamsitnik adamsitnik added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed untriaged New issue has not been triaged by the area owner labels Nov 26, 2020
@Tyrrrz
Copy link
Contributor

Tyrrrz commented Feb 8, 2021

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
Copy link

I am affected by this as well in a very similar way to the above mentioned issue (pulumi/pulumi-dotnet#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
Copy link

Any updates?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Diagnostics.Process needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Projects
None yet
Development

No branches or pull requests

8 participants