Introduce SafeProcessHandle.Start and ProcessId - #126192

Merged
adamsitnik merged 8 commits into
dotnet:mainfrom
adamsitnik:sph-start
Mar 30, 2026
Merged

Introduce SafeProcessHandle.Start and ProcessId#126192
adamsitnik merged 8 commits into
dotnet:mainfrom
adamsitnik:sph-start

Conversation

@adamsitnik

@adamsitnikadamsitnik commented Mar 27, 2026

Copy link
Copy Markdown
Member

This is one of very few PRs that I recently wrote myself without using AI. The reason is that I've failed to write the right prompt and TBH and I was also afraid of some subtle hallucinations being introduced.

I've just moved all the helpers from Process.[Unix/Win32/Windows].cs into ProcessUtils.$platform.cs and moved the logic responsible for launching processes with given handles to SafeProcessHandle.StartCore.

First two commits are more or less cut and paste.

The third commit is most important: 0028e06 as it contains some non-obvious changes (see my other comments).

The fourth commit is just exposing ProcessId.

The fifth commit are some basic tests.

The API was approved in #125838 (comment).

This PR will allow me to implement the new "scenario" methods without using Process directly and referencing a tone of code. Example: #126078

Once this is merged, I am going to send other PR:

  • SafeProcessHandle.Kill and Signal
  • SafeProcessHandle.WaitForExit*

fixes#126107

@adamsitnikadamsitnik added the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 27, 2026
CopilotAI review requested due to automatic review settings March 27, 2026 09:47
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 22 out of 22 changed files in this pull request and generated no comments.

@adamsitnik
adamsitnikforce-pushed the sph-start branch 2 times, most recently from 2d1b324 to c2eda1dCompareMarch 27, 2026 14:39
CopilotAI review requested due to automatic review settings March 27, 2026 14:39

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.

@adamsitnikadamsitnik removed the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 27, 2026
@adamsitnikadamsitnik changed the title Introduce SafeProcessHandle.StartIntroduce SafeProcessHandle.Start and ProcessIdMar 27, 2026
@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@copilot invoke the code-review skill and post the analysis/results as a comment on this PR

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.

Comment threadsrc/libraries/System.Diagnostics.Process/tests/SafeProcessHandleTests.cs Outdated
Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
The ProcessWaitState.Holder was created outside the read lock in the
refactored ForkAndExecProcess, introducing a race condition:
1. Fork+exec child (inside read lock)
2. Release read lock
3. SIGCHLD fires → OnSigChild takes write lock → CheckChildren runs
4. Child is NOT yet in s_childProcessWaitStates → SIGCHLD is consumed
without reaping the child
5. ProcessWaitState.Holder created (too late)
6. WaitForExit waits on _exitedEvent that will never be set → HANG
The fix mirrors the original Process.Unix.cs code: create the
ProcessWaitState.Holder while still holding the read lock, so the
child is in s_childProcessWaitStates before SIGCHLD can be processed.
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/325db55f-9e1f-43bd-abec-1b60c0b2b5ce
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
@tmds

tmds commented Mar 30, 2026

Copy link
Copy Markdown
Member

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@rokonec@eiriktsarpalis Could you please review the Windows part? In the first commit (968d39a) I've just moved the code to other files to allow for exposing new APIs that don't have a dependency on Process. I've added GH comments in places where I've introduced changes to Windows implementation.

@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

I've not investigated it myself (yet).

@rokonecrokonec left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@adamsitnik
adamsitnik merged commit dee9005 into dotnet:mainMar 30, 2026
93 of 97 checks passed

@eiriktsarpaliseiriktsarpalis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few minor post-merge comments:


if (!startInfo.UseShellExecute)
{
if (childInputHandle is null && !OperatingSystem.IsAndroid())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Out of curiosity, what's specific about android in this check?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Android does not support Console.OpenStandard[Input/Output/Errror]Handle methods as it does not support Console, but it still has some standard input/output/error handles, so we just pass null here and the native layer takes care of it.

return true;
}

// On Unix, we don't use process descriptors yet, so we can't get PID.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does "yet" mean in this context? Is it something we haven't implemented in the PAL? Do we need to link to an open issue?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I intend to extend SafeProcessHandle with process descriptors support for 11, but just not in this PR.

public sealed partial class SafeProcessHandle : SafeHandleZeroOrMinusOneIsInvalid
{
internal static readonly SafeProcessHandle InvalidHandle = new SafeProcessHandle();
private int _processId = -1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems like a prime use case for using the new field keyword.

: new("sh") { ArgumentList = { "-c", "exit 42" } };

using SafeProcessHandle processHandle = SafeProcessHandle.Start(startInfo);
Assert.NotEqual(0, processHandle.ProcessId);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should there be a test checking for the validation code in this property getter?

@jkotas

Copy link
Copy Markdown
Member

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

It always drags in a lot of performance counter related functionality and the dependencies are not straightforward to break without observable behavior changes. (It may be possible to improve this, it is just not straightforward.)

For example, here is a dependency chain from https://github.com/MichalStrehovsky/sizoscope for a simple app that just does Process.Start("notepad.exe").WaitForExit()

image

@jkotas

Copy link
Copy Markdown
Member

Opened #126332

adamsitnik added a commit that referenced this pull request Mar 31, 2026
…te) (#126314)
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 30, 2026
Sign up for freeto 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.

Implement SafeProcessHandle.Start

7 participants

@adamsitnik@tmds@jkotas@eiriktsarpalis@rokonec
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Introduce SafeProcessHandle.Start and ProcessId - #126192

Merged
adamsitnik merged 8 commits into
dotnet:mainfrom
adamsitnik:sph-start
Mar 30, 2026
Merged

Introduce SafeProcessHandle.Start and ProcessId#126192
adamsitnik merged 8 commits into
dotnet:mainfrom
adamsitnik:sph-start

Conversation

@adamsitnik

@adamsitnikadamsitnik commented Mar 27, 2026

Copy link
Copy Markdown
Member

This is one of very few PRs that I recently wrote myself without using AI. The reason is that I've failed to write the right prompt and TBH and I was also afraid of some subtle hallucinations being introduced.

I've just moved all the helpers from Process.[Unix/Win32/Windows].cs into ProcessUtils.$platform.cs and moved the logic responsible for launching processes with given handles to SafeProcessHandle.StartCore.

First two commits are more or less cut and paste.

The third commit is most important: 0028e06 as it contains some non-obvious changes (see my other comments).

The fourth commit is just exposing ProcessId.

The fifth commit are some basic tests.

The API was approved in #125838 (comment).

This PR will allow me to implement the new "scenario" methods without using Process directly and referencing a tone of code. Example: #126078

Once this is merged, I am going to send other PR:

  • SafeProcessHandle.Kill and Signal
  • SafeProcessHandle.WaitForExit*

fixes#126107

@adamsitnikadamsitnik added the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 27, 2026
CopilotAI review requested due to automatic review settings March 27, 2026 09:47
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 22 out of 22 changed files in this pull request and generated no comments.

@adamsitnik
adamsitnikforce-pushed the sph-start branch 2 times, most recently from 2d1b324 to c2eda1dCompareMarch 27, 2026 14:39
CopilotAI review requested due to automatic review settings March 27, 2026 14:39

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.

@adamsitnikadamsitnik removed the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 27, 2026
@adamsitnikadamsitnik changed the title Introduce SafeProcessHandle.StartIntroduce SafeProcessHandle.Start and ProcessIdMar 27, 2026
@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@copilot invoke the code-review skill and post the analysis/results as a comment on this PR

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.

Comment threadsrc/libraries/System.Diagnostics.Process/tests/SafeProcessHandleTests.cs Outdated
Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
The ProcessWaitState.Holder was created outside the read lock in the
refactored ForkAndExecProcess, introducing a race condition:
1. Fork+exec child (inside read lock)
2. Release read lock
3. SIGCHLD fires → OnSigChild takes write lock → CheckChildren runs
4. Child is NOT yet in s_childProcessWaitStates → SIGCHLD is consumed
without reaping the child
5. ProcessWaitState.Holder created (too late)
6. WaitForExit waits on _exitedEvent that will never be set → HANG
The fix mirrors the original Process.Unix.cs code: create the
ProcessWaitState.Holder while still holding the read lock, so the
child is in s_childProcessWaitStates before SIGCHLD can be processed.
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/325db55f-9e1f-43bd-abec-1b60c0b2b5ce
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
@tmds

tmds commented Mar 30, 2026

Copy link
Copy Markdown
Member

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@rokonec@eiriktsarpalis Could you please review the Windows part? In the first commit (968d39a) I've just moved the code to other files to allow for exposing new APIs that don't have a dependency on Process. I've added GH comments in places where I've introduced changes to Windows implementation.

@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

I've not investigated it myself (yet).

@rokonecrokonec left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@adamsitnik
adamsitnik merged commit dee9005 into dotnet:mainMar 30, 2026
93 of 97 checks passed

@eiriktsarpaliseiriktsarpalis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few minor post-merge comments:


if (!startInfo.UseShellExecute)
{
if (childInputHandle is null && !OperatingSystem.IsAndroid())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Out of curiosity, what's specific about android in this check?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Android does not support Console.OpenStandard[Input/Output/Errror]Handle methods as it does not support Console, but it still has some standard input/output/error handles, so we just pass null here and the native layer takes care of it.

return true;
}

// On Unix, we don't use process descriptors yet, so we can't get PID.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does "yet" mean in this context? Is it something we haven't implemented in the PAL? Do we need to link to an open issue?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I intend to extend SafeProcessHandle with process descriptors support for 11, but just not in this PR.

public sealed partial class SafeProcessHandle : SafeHandleZeroOrMinusOneIsInvalid
{
internal static readonly SafeProcessHandle InvalidHandle = new SafeProcessHandle();
private int _processId = -1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems like a prime use case for using the new field keyword.

: new("sh") { ArgumentList = { "-c", "exit 42" } };

using SafeProcessHandle processHandle = SafeProcessHandle.Start(startInfo);
Assert.NotEqual(0, processHandle.ProcessId);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should there be a test checking for the validation code in this property getter?

@jkotas

Copy link
Copy Markdown
Member

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

It always drags in a lot of performance counter related functionality and the dependencies are not straightforward to break without observable behavior changes. (It may be possible to improve this, it is just not straightforward.)

For example, here is a dependency chain from https://github.com/MichalStrehovsky/sizoscope for a simple app that just does Process.Start("notepad.exe").WaitForExit()

image

@jkotas

Copy link
Copy Markdown
Member

Opened #126332

adamsitnik added a commit that referenced this pull request Mar 31, 2026
…te) (#126314)
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 30, 2026
Sign up for freeto 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.

Implement SafeProcessHandle.Start

7 participants

@adamsitnik@tmds@jkotas@eiriktsarpalis@rokonec
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Introduce SafeProcessHandle.Start and ProcessId - #126192

Merged
adamsitnik merged 8 commits into
dotnet:mainfrom
adamsitnik:sph-start
Mar 30, 2026
Merged

Introduce SafeProcessHandle.Start and ProcessId#126192
adamsitnik merged 8 commits into
dotnet:mainfrom
adamsitnik:sph-start

Conversation

@adamsitnik

@adamsitnikadamsitnik commented Mar 27, 2026

Copy link
Copy Markdown
Member

This is one of very few PRs that I recently wrote myself without using AI. The reason is that I've failed to write the right prompt and TBH and I was also afraid of some subtle hallucinations being introduced.

I've just moved all the helpers from Process.[Unix/Win32/Windows].cs into ProcessUtils.$platform.cs and moved the logic responsible for launching processes with given handles to SafeProcessHandle.StartCore.

First two commits are more or less cut and paste.

The third commit is most important: 0028e06 as it contains some non-obvious changes (see my other comments).

The fourth commit is just exposing ProcessId.

The fifth commit are some basic tests.

The API was approved in #125838 (comment).

This PR will allow me to implement the new "scenario" methods without using Process directly and referencing a tone of code. Example: #126078

Once this is merged, I am going to send other PR:

  • SafeProcessHandle.Kill and Signal
  • SafeProcessHandle.WaitForExit*

fixes#126107

@adamsitnikadamsitnik added the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 27, 2026
CopilotAI review requested due to automatic review settings March 27, 2026 09:47
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 22 out of 22 changed files in this pull request and generated no comments.

@adamsitnik
adamsitnikforce-pushed the sph-start branch 2 times, most recently from 2d1b324 to c2eda1dCompareMarch 27, 2026 14:39
CopilotAI review requested due to automatic review settings March 27, 2026 14:39

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.

@adamsitnikadamsitnik removed the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 27, 2026
@adamsitnikadamsitnik changed the title Introduce SafeProcessHandle.StartIntroduce SafeProcessHandle.Start and ProcessIdMar 27, 2026
@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@copilot invoke the code-review skill and post the analysis/results as a comment on this PR

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.

Comment threadsrc/libraries/System.Diagnostics.Process/tests/SafeProcessHandleTests.cs Outdated
Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
The ProcessWaitState.Holder was created outside the read lock in the
refactored ForkAndExecProcess, introducing a race condition:
1. Fork+exec child (inside read lock)
2. Release read lock
3. SIGCHLD fires → OnSigChild takes write lock → CheckChildren runs
4. Child is NOT yet in s_childProcessWaitStates → SIGCHLD is consumed
without reaping the child
5. ProcessWaitState.Holder created (too late)
6. WaitForExit waits on _exitedEvent that will never be set → HANG
The fix mirrors the original Process.Unix.cs code: create the
ProcessWaitState.Holder while still holding the read lock, so the
child is in s_childProcessWaitStates before SIGCHLD can be processed.
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/325db55f-9e1f-43bd-abec-1b60c0b2b5ce
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
@tmds

tmds commented Mar 30, 2026

Copy link
Copy Markdown
Member

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@rokonec@eiriktsarpalis Could you please review the Windows part? In the first commit (968d39a) I've just moved the code to other files to allow for exposing new APIs that don't have a dependency on Process. I've added GH comments in places where I've introduced changes to Windows implementation.

@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

I've not investigated it myself (yet).

@rokonecrokonec left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@adamsitnik
adamsitnik merged commit dee9005 into dotnet:mainMar 30, 2026
93 of 97 checks passed

@eiriktsarpaliseiriktsarpalis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few minor post-merge comments:


if (!startInfo.UseShellExecute)
{
if (childInputHandle is null && !OperatingSystem.IsAndroid())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Out of curiosity, what's specific about android in this check?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Android does not support Console.OpenStandard[Input/Output/Errror]Handle methods as it does not support Console, but it still has some standard input/output/error handles, so we just pass null here and the native layer takes care of it.

return true;
}

// On Unix, we don't use process descriptors yet, so we can't get PID.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does "yet" mean in this context? Is it something we haven't implemented in the PAL? Do we need to link to an open issue?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I intend to extend SafeProcessHandle with process descriptors support for 11, but just not in this PR.

public sealed partial class SafeProcessHandle : SafeHandleZeroOrMinusOneIsInvalid
{
internal static readonly SafeProcessHandle InvalidHandle = new SafeProcessHandle();
private int _processId = -1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems like a prime use case for using the new field keyword.

: new("sh") { ArgumentList = { "-c", "exit 42" } };

using SafeProcessHandle processHandle = SafeProcessHandle.Start(startInfo);
Assert.NotEqual(0, processHandle.ProcessId);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should there be a test checking for the validation code in this property getter?

@jkotas

Copy link
Copy Markdown
Member

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

It always drags in a lot of performance counter related functionality and the dependencies are not straightforward to break without observable behavior changes. (It may be possible to improve this, it is just not straightforward.)

For example, here is a dependency chain from https://github.com/MichalStrehovsky/sizoscope for a simple app that just does Process.Start("notepad.exe").WaitForExit()

image

@jkotas

Copy link
Copy Markdown
Member

Opened #126332

adamsitnik added a commit that referenced this pull request Mar 31, 2026
…te) (#126314)
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 30, 2026
Sign up for freeto 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.

Implement SafeProcessHandle.Start

7 participants

@adamsitnik@tmds@jkotas@eiriktsarpalis@rokonec
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Introduce SafeProcessHandle.Start and ProcessId - #126192

Merged
adamsitnik merged 8 commits into
dotnet:mainfrom
adamsitnik:sph-start
Mar 30, 2026
Merged

Introduce SafeProcessHandle.Start and ProcessId#126192
adamsitnik merged 8 commits into
dotnet:mainfrom
adamsitnik:sph-start

Conversation

@adamsitnik

@adamsitnikadamsitnik commented Mar 27, 2026

Copy link
Copy Markdown
Member

This is one of very few PRs that I recently wrote myself without using AI. The reason is that I've failed to write the right prompt and TBH and I was also afraid of some subtle hallucinations being introduced.

I've just moved all the helpers from Process.[Unix/Win32/Windows].cs into ProcessUtils.$platform.cs and moved the logic responsible for launching processes with given handles to SafeProcessHandle.StartCore.

First two commits are more or less cut and paste.

The third commit is most important: 0028e06 as it contains some non-obvious changes (see my other comments).

The fourth commit is just exposing ProcessId.

The fifth commit are some basic tests.

The API was approved in #125838 (comment).

This PR will allow me to implement the new "scenario" methods without using Process directly and referencing a tone of code. Example: #126078

Once this is merged, I am going to send other PR:

  • SafeProcessHandle.Kill and Signal
  • SafeProcessHandle.WaitForExit*

fixes#126107

@adamsitnikadamsitnik added the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 27, 2026
CopilotAI review requested due to automatic review settings March 27, 2026 09:47
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 22 out of 22 changed files in this pull request and generated no comments.

@adamsitnik
adamsitnikforce-pushed the sph-start branch 2 times, most recently from 2d1b324 to c2eda1dCompareMarch 27, 2026 14:39
CopilotAI review requested due to automatic review settings March 27, 2026 14:39

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.

@adamsitnikadamsitnik removed the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 27, 2026
@adamsitnikadamsitnik changed the title Introduce SafeProcessHandle.StartIntroduce SafeProcessHandle.Start and ProcessIdMar 27, 2026
@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@copilot invoke the code-review skill and post the analysis/results as a comment on this PR

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.

Comment threadsrc/libraries/System.Diagnostics.Process/tests/SafeProcessHandleTests.cs Outdated
Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
The ProcessWaitState.Holder was created outside the read lock in the
refactored ForkAndExecProcess, introducing a race condition:
1. Fork+exec child (inside read lock)
2. Release read lock
3. SIGCHLD fires → OnSigChild takes write lock → CheckChildren runs
4. Child is NOT yet in s_childProcessWaitStates → SIGCHLD is consumed
without reaping the child
5. ProcessWaitState.Holder created (too late)
6. WaitForExit waits on _exitedEvent that will never be set → HANG
The fix mirrors the original Process.Unix.cs code: create the
ProcessWaitState.Holder while still holding the read lock, so the
child is in s_childProcessWaitStates before SIGCHLD can be processed.
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/325db55f-9e1f-43bd-abec-1b60c0b2b5ce
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
@tmds

tmds commented Mar 30, 2026

Copy link
Copy Markdown
Member

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@rokonec@eiriktsarpalis Could you please review the Windows part? In the first commit (968d39a) I've just moved the code to other files to allow for exposing new APIs that don't have a dependency on Process. I've added GH comments in places where I've introduced changes to Windows implementation.

@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

I've not investigated it myself (yet).

@rokonecrokonec left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@adamsitnik
adamsitnik merged commit dee9005 into dotnet:mainMar 30, 2026
93 of 97 checks passed

@eiriktsarpaliseiriktsarpalis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few minor post-merge comments:


if (!startInfo.UseShellExecute)
{
if (childInputHandle is null && !OperatingSystem.IsAndroid())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Out of curiosity, what's specific about android in this check?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Android does not support Console.OpenStandard[Input/Output/Errror]Handle methods as it does not support Console, but it still has some standard input/output/error handles, so we just pass null here and the native layer takes care of it.

return true;
}

// On Unix, we don't use process descriptors yet, so we can't get PID.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does "yet" mean in this context? Is it something we haven't implemented in the PAL? Do we need to link to an open issue?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I intend to extend SafeProcessHandle with process descriptors support for 11, but just not in this PR.

public sealed partial class SafeProcessHandle : SafeHandleZeroOrMinusOneIsInvalid
{
internal static readonly SafeProcessHandle InvalidHandle = new SafeProcessHandle();
private int _processId = -1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems like a prime use case for using the new field keyword.

: new("sh") { ArgumentList = { "-c", "exit 42" } };

using SafeProcessHandle processHandle = SafeProcessHandle.Start(startInfo);
Assert.NotEqual(0, processHandle.ProcessId);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should there be a test checking for the validation code in this property getter?

@jkotas

Copy link
Copy Markdown
Member

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

It always drags in a lot of performance counter related functionality and the dependencies are not straightforward to break without observable behavior changes. (It may be possible to improve this, it is just not straightforward.)

For example, here is a dependency chain from https://github.com/MichalStrehovsky/sizoscope for a simple app that just does Process.Start("notepad.exe").WaitForExit()

image

@jkotas

Copy link
Copy Markdown
Member

Opened #126332

adamsitnik added a commit that referenced this pull request Mar 31, 2026
…te) (#126314)
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 30, 2026
Sign up for freeto 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.

Implement SafeProcessHandle.Start

7 participants

@adamsitnik@tmds@jkotas@eiriktsarpalis@rokonec
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Introduce SafeProcessHandle.Start and ProcessId - #126192

Merged
adamsitnik merged 8 commits into
dotnet:mainfrom
adamsitnik:sph-start
Mar 30, 2026
Merged

Introduce SafeProcessHandle.Start and ProcessId#126192
adamsitnik merged 8 commits into
dotnet:mainfrom
adamsitnik:sph-start

Conversation

@adamsitnik

@adamsitnikadamsitnik commented Mar 27, 2026

Copy link
Copy Markdown
Member

This is one of very few PRs that I recently wrote myself without using AI. The reason is that I've failed to write the right prompt and TBH and I was also afraid of some subtle hallucinations being introduced.

I've just moved all the helpers from Process.[Unix/Win32/Windows].cs into ProcessUtils.$platform.cs and moved the logic responsible for launching processes with given handles to SafeProcessHandle.StartCore.

First two commits are more or less cut and paste.

The third commit is most important: 0028e06 as it contains some non-obvious changes (see my other comments).

The fourth commit is just exposing ProcessId.

The fifth commit are some basic tests.

The API was approved in #125838 (comment).

This PR will allow me to implement the new "scenario" methods without using Process directly and referencing a tone of code. Example: #126078

Once this is merged, I am going to send other PR:

  • SafeProcessHandle.Kill and Signal
  • SafeProcessHandle.WaitForExit*

fixes#126107

@adamsitnikadamsitnik added the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 27, 2026
CopilotAI review requested due to automatic review settings March 27, 2026 09:47
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 22 out of 22 changed files in this pull request and generated no comments.

@adamsitnik
adamsitnikforce-pushed the sph-start branch 2 times, most recently from 2d1b324 to c2eda1dCompareMarch 27, 2026 14:39
CopilotAI review requested due to automatic review settings March 27, 2026 14:39

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.

@adamsitnikadamsitnik removed the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 27, 2026
@adamsitnikadamsitnik changed the title Introduce SafeProcessHandle.StartIntroduce SafeProcessHandle.Start and ProcessIdMar 27, 2026
@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@copilot invoke the code-review skill and post the analysis/results as a comment on this PR

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.

Comment threadsrc/libraries/System.Diagnostics.Process/tests/SafeProcessHandleTests.cs Outdated
Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
The ProcessWaitState.Holder was created outside the read lock in the
refactored ForkAndExecProcess, introducing a race condition:
1. Fork+exec child (inside read lock)
2. Release read lock
3. SIGCHLD fires → OnSigChild takes write lock → CheckChildren runs
4. Child is NOT yet in s_childProcessWaitStates → SIGCHLD is consumed
without reaping the child
5. ProcessWaitState.Holder created (too late)
6. WaitForExit waits on _exitedEvent that will never be set → HANG
The fix mirrors the original Process.Unix.cs code: create the
ProcessWaitState.Holder while still holding the read lock, so the
child is in s_childProcessWaitStates before SIGCHLD can be processed.
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/325db55f-9e1f-43bd-abec-1b60c0b2b5ce
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
@tmds

tmds commented Mar 30, 2026

Copy link
Copy Markdown
Member

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@rokonec@eiriktsarpalis Could you please review the Windows part? In the first commit (968d39a) I've just moved the code to other files to allow for exposing new APIs that don't have a dependency on Process. I've added GH comments in places where I've introduced changes to Windows implementation.

@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

I've not investigated it myself (yet).

@rokonecrokonec left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@adamsitnik
adamsitnik merged commit dee9005 into dotnet:mainMar 30, 2026
93 of 97 checks passed

@eiriktsarpaliseiriktsarpalis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few minor post-merge comments:


if (!startInfo.UseShellExecute)
{
if (childInputHandle is null && !OperatingSystem.IsAndroid())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Out of curiosity, what's specific about android in this check?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Android does not support Console.OpenStandard[Input/Output/Errror]Handle methods as it does not support Console, but it still has some standard input/output/error handles, so we just pass null here and the native layer takes care of it.

return true;
}

// On Unix, we don't use process descriptors yet, so we can't get PID.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does "yet" mean in this context? Is it something we haven't implemented in the PAL? Do we need to link to an open issue?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I intend to extend SafeProcessHandle with process descriptors support for 11, but just not in this PR.

public sealed partial class SafeProcessHandle : SafeHandleZeroOrMinusOneIsInvalid
{
internal static readonly SafeProcessHandle InvalidHandle = new SafeProcessHandle();
private int _processId = -1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems like a prime use case for using the new field keyword.

: new("sh") { ArgumentList = { "-c", "exit 42" } };

using SafeProcessHandle processHandle = SafeProcessHandle.Start(startInfo);
Assert.NotEqual(0, processHandle.ProcessId);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should there be a test checking for the validation code in this property getter?

@jkotas

Copy link
Copy Markdown
Member

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

It always drags in a lot of performance counter related functionality and the dependencies are not straightforward to break without observable behavior changes. (It may be possible to improve this, it is just not straightforward.)

For example, here is a dependency chain from https://github.com/MichalStrehovsky/sizoscope for a simple app that just does Process.Start("notepad.exe").WaitForExit()

image

@jkotas

Copy link
Copy Markdown
Member

Opened #126332

adamsitnik added a commit that referenced this pull request Mar 31, 2026
…te) (#126314)
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 30, 2026
Sign up for freeto 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.

Implement SafeProcessHandle.Start

7 participants

@adamsitnik@tmds@jkotas@eiriktsarpalis@rokonec
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Introduce SafeProcessHandle.Start and ProcessId - #126192

Merged
adamsitnik merged 8 commits into
dotnet:mainfrom
adamsitnik:sph-start
Mar 30, 2026
Merged

Introduce SafeProcessHandle.Start and ProcessId#126192
adamsitnik merged 8 commits into
dotnet:mainfrom
adamsitnik:sph-start

Conversation

@adamsitnik

@adamsitnikadamsitnik commented Mar 27, 2026

Copy link
Copy Markdown
Member

This is one of very few PRs that I recently wrote myself without using AI. The reason is that I've failed to write the right prompt and TBH and I was also afraid of some subtle hallucinations being introduced.

I've just moved all the helpers from Process.[Unix/Win32/Windows].cs into ProcessUtils.$platform.cs and moved the logic responsible for launching processes with given handles to SafeProcessHandle.StartCore.

First two commits are more or less cut and paste.

The third commit is most important: 0028e06 as it contains some non-obvious changes (see my other comments).

The fourth commit is just exposing ProcessId.

The fifth commit are some basic tests.

The API was approved in #125838 (comment).

This PR will allow me to implement the new "scenario" methods without using Process directly and referencing a tone of code. Example: #126078

Once this is merged, I am going to send other PR:

  • SafeProcessHandle.Kill and Signal
  • SafeProcessHandle.WaitForExit*

fixes#126107

@adamsitnikadamsitnik added the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 27, 2026
CopilotAI review requested due to automatic review settings March 27, 2026 09:47
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 22 out of 22 changed files in this pull request and generated no comments.

@adamsitnik
adamsitnikforce-pushed the sph-start branch 2 times, most recently from 2d1b324 to c2eda1dCompareMarch 27, 2026 14:39
CopilotAI review requested due to automatic review settings March 27, 2026 14:39

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.

@adamsitnikadamsitnik removed the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 27, 2026
@adamsitnikadamsitnik changed the title Introduce SafeProcessHandle.StartIntroduce SafeProcessHandle.Start and ProcessIdMar 27, 2026
@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@copilot invoke the code-review skill and post the analysis/results as a comment on this PR

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.

Comment threadsrc/libraries/System.Diagnostics.Process/tests/SafeProcessHandleTests.cs Outdated
Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
The ProcessWaitState.Holder was created outside the read lock in the
refactored ForkAndExecProcess, introducing a race condition:
1. Fork+exec child (inside read lock)
2. Release read lock
3. SIGCHLD fires → OnSigChild takes write lock → CheckChildren runs
4. Child is NOT yet in s_childProcessWaitStates → SIGCHLD is consumed
without reaping the child
5. ProcessWaitState.Holder created (too late)
6. WaitForExit waits on _exitedEvent that will never be set → HANG
The fix mirrors the original Process.Unix.cs code: create the
ProcessWaitState.Holder while still holding the read lock, so the
child is in s_childProcessWaitStates before SIGCHLD can be processed.
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/325db55f-9e1f-43bd-abec-1b60c0b2b5ce
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
@tmds

tmds commented Mar 30, 2026

Copy link
Copy Markdown
Member

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@rokonec@eiriktsarpalis Could you please review the Windows part? In the first commit (968d39a) I've just moved the code to other files to allow for exposing new APIs that don't have a dependency on Process. I've added GH comments in places where I've introduced changes to Windows implementation.

@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

I've not investigated it myself (yet).

@rokonecrokonec left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@adamsitnik
adamsitnik merged commit dee9005 into dotnet:mainMar 30, 2026
93 of 97 checks passed

@eiriktsarpaliseiriktsarpalis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few minor post-merge comments:


if (!startInfo.UseShellExecute)
{
if (childInputHandle is null && !OperatingSystem.IsAndroid())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Out of curiosity, what's specific about android in this check?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Android does not support Console.OpenStandard[Input/Output/Errror]Handle methods as it does not support Console, but it still has some standard input/output/error handles, so we just pass null here and the native layer takes care of it.

return true;
}

// On Unix, we don't use process descriptors yet, so we can't get PID.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does "yet" mean in this context? Is it something we haven't implemented in the PAL? Do we need to link to an open issue?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I intend to extend SafeProcessHandle with process descriptors support for 11, but just not in this PR.

public sealed partial class SafeProcessHandle : SafeHandleZeroOrMinusOneIsInvalid
{
internal static readonly SafeProcessHandle InvalidHandle = new SafeProcessHandle();
private int _processId = -1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems like a prime use case for using the new field keyword.

: new("sh") { ArgumentList = { "-c", "exit 42" } };

using SafeProcessHandle processHandle = SafeProcessHandle.Start(startInfo);
Assert.NotEqual(0, processHandle.ProcessId);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should there be a test checking for the validation code in this property getter?

@jkotas

Copy link
Copy Markdown
Member

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

It always drags in a lot of performance counter related functionality and the dependencies are not straightforward to break without observable behavior changes. (It may be possible to improve this, it is just not straightforward.)

For example, here is a dependency chain from https://github.com/MichalStrehovsky/sizoscope for a simple app that just does Process.Start("notepad.exe").WaitForExit()

image

@jkotas

Copy link
Copy Markdown
Member

Opened #126332

adamsitnik added a commit that referenced this pull request Mar 31, 2026
…te) (#126314)
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 30, 2026
Sign up for freeto 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.

Implement SafeProcessHandle.Start

7 participants

@adamsitnik@tmds@jkotas@eiriktsarpalis@rokonec
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Introduce SafeProcessHandle.Start and ProcessId - #126192

Merged
adamsitnik merged 8 commits into
dotnet:mainfrom
adamsitnik:sph-start
Mar 30, 2026
Merged

Introduce SafeProcessHandle.Start and ProcessId#126192
adamsitnik merged 8 commits into
dotnet:mainfrom
adamsitnik:sph-start

Conversation

@adamsitnik

@adamsitnikadamsitnik commented Mar 27, 2026

Copy link
Copy Markdown
Member

This is one of very few PRs that I recently wrote myself without using AI. The reason is that I've failed to write the right prompt and TBH and I was also afraid of some subtle hallucinations being introduced.

I've just moved all the helpers from Process.[Unix/Win32/Windows].cs into ProcessUtils.$platform.cs and moved the logic responsible for launching processes with given handles to SafeProcessHandle.StartCore.

First two commits are more or less cut and paste.

The third commit is most important: 0028e06 as it contains some non-obvious changes (see my other comments).

The fourth commit is just exposing ProcessId.

The fifth commit are some basic tests.

The API was approved in #125838 (comment).

This PR will allow me to implement the new "scenario" methods without using Process directly and referencing a tone of code. Example: #126078

Once this is merged, I am going to send other PR:

  • SafeProcessHandle.Kill and Signal
  • SafeProcessHandle.WaitForExit*

fixes#126107

@adamsitnikadamsitnik added the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 27, 2026
CopilotAI review requested due to automatic review settings March 27, 2026 09:47
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 22 out of 22 changed files in this pull request and generated no comments.

@adamsitnik
adamsitnikforce-pushed the sph-start branch 2 times, most recently from 2d1b324 to c2eda1dCompareMarch 27, 2026 14:39
CopilotAI review requested due to automatic review settings March 27, 2026 14:39

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.

@adamsitnikadamsitnik removed the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 27, 2026
@adamsitnikadamsitnik changed the title Introduce SafeProcessHandle.StartIntroduce SafeProcessHandle.Start and ProcessIdMar 27, 2026
@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@copilot invoke the code-review skill and post the analysis/results as a comment on this PR

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.

Comment threadsrc/libraries/System.Diagnostics.Process/tests/SafeProcessHandleTests.cs Outdated
Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
The ProcessWaitState.Holder was created outside the read lock in the
refactored ForkAndExecProcess, introducing a race condition:
1. Fork+exec child (inside read lock)
2. Release read lock
3. SIGCHLD fires → OnSigChild takes write lock → CheckChildren runs
4. Child is NOT yet in s_childProcessWaitStates → SIGCHLD is consumed
without reaping the child
5. ProcessWaitState.Holder created (too late)
6. WaitForExit waits on _exitedEvent that will never be set → HANG
The fix mirrors the original Process.Unix.cs code: create the
ProcessWaitState.Holder while still holding the read lock, so the
child is in s_childProcessWaitStates before SIGCHLD can be processed.
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/325db55f-9e1f-43bd-abec-1b60c0b2b5ce
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
@tmds

tmds commented Mar 30, 2026

Copy link
Copy Markdown
Member

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@rokonec@eiriktsarpalis Could you please review the Windows part? In the first commit (968d39a) I've just moved the code to other files to allow for exposing new APIs that don't have a dependency on Process. I've added GH comments in places where I've introduced changes to Windows implementation.

@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

I've not investigated it myself (yet).

@rokonecrokonec left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@adamsitnik
adamsitnik merged commit dee9005 into dotnet:mainMar 30, 2026
93 of 97 checks passed

@eiriktsarpaliseiriktsarpalis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few minor post-merge comments:


if (!startInfo.UseShellExecute)
{
if (childInputHandle is null && !OperatingSystem.IsAndroid())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Out of curiosity, what's specific about android in this check?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Android does not support Console.OpenStandard[Input/Output/Errror]Handle methods as it does not support Console, but it still has some standard input/output/error handles, so we just pass null here and the native layer takes care of it.

return true;
}

// On Unix, we don't use process descriptors yet, so we can't get PID.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does "yet" mean in this context? Is it something we haven't implemented in the PAL? Do we need to link to an open issue?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I intend to extend SafeProcessHandle with process descriptors support for 11, but just not in this PR.

public sealed partial class SafeProcessHandle : SafeHandleZeroOrMinusOneIsInvalid
{
internal static readonly SafeProcessHandle InvalidHandle = new SafeProcessHandle();
private int _processId = -1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems like a prime use case for using the new field keyword.

: new("sh") { ArgumentList = { "-c", "exit 42" } };

using SafeProcessHandle processHandle = SafeProcessHandle.Start(startInfo);
Assert.NotEqual(0, processHandle.ProcessId);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should there be a test checking for the validation code in this property getter?

@jkotas

Copy link
Copy Markdown
Member

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

It always drags in a lot of performance counter related functionality and the dependencies are not straightforward to break without observable behavior changes. (It may be possible to improve this, it is just not straightforward.)

For example, here is a dependency chain from https://github.com/MichalStrehovsky/sizoscope for a simple app that just does Process.Start("notepad.exe").WaitForExit()

image

@jkotas

Copy link
Copy Markdown
Member

Opened #126332

adamsitnik added a commit that referenced this pull request Mar 31, 2026
…te) (#126314)
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 30, 2026
Sign up for freeto 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.

Implement SafeProcessHandle.Start

7 participants

@adamsitnik@tmds@jkotas@eiriktsarpalis@rokonec
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Introduce SafeProcessHandle.Start and ProcessId - #126192

Merged
adamsitnik merged 8 commits into
dotnet:mainfrom
adamsitnik:sph-start
Mar 30, 2026
Merged

Introduce SafeProcessHandle.Start and ProcessId#126192
adamsitnik merged 8 commits into
dotnet:mainfrom
adamsitnik:sph-start

Conversation

@adamsitnik

@adamsitnikadamsitnik commented Mar 27, 2026

Copy link
Copy Markdown
Member

This is one of very few PRs that I recently wrote myself without using AI. The reason is that I've failed to write the right prompt and TBH and I was also afraid of some subtle hallucinations being introduced.

I've just moved all the helpers from Process.[Unix/Win32/Windows].cs into ProcessUtils.$platform.cs and moved the logic responsible for launching processes with given handles to SafeProcessHandle.StartCore.

First two commits are more or less cut and paste.

The third commit is most important: 0028e06 as it contains some non-obvious changes (see my other comments).

The fourth commit is just exposing ProcessId.

The fifth commit are some basic tests.

The API was approved in #125838 (comment).

This PR will allow me to implement the new "scenario" methods without using Process directly and referencing a tone of code. Example: #126078

Once this is merged, I am going to send other PR:

  • SafeProcessHandle.Kill and Signal
  • SafeProcessHandle.WaitForExit*

fixes#126107

@adamsitnikadamsitnik added the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 27, 2026
CopilotAI review requested due to automatic review settings March 27, 2026 09:47
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 22 out of 22 changed files in this pull request and generated no comments.

@adamsitnik
adamsitnikforce-pushed the sph-start branch 2 times, most recently from 2d1b324 to c2eda1dCompareMarch 27, 2026 14:39
CopilotAI review requested due to automatic review settings March 27, 2026 14:39

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.

@adamsitnikadamsitnik removed the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 27, 2026
@adamsitnikadamsitnik changed the title Introduce SafeProcessHandle.StartIntroduce SafeProcessHandle.Start and ProcessIdMar 27, 2026
@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@copilot invoke the code-review skill and post the analysis/results as a comment on this PR

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.

Comment threadsrc/libraries/System.Diagnostics.Process/tests/SafeProcessHandleTests.cs Outdated
Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
The ProcessWaitState.Holder was created outside the read lock in the
refactored ForkAndExecProcess, introducing a race condition:
1. Fork+exec child (inside read lock)
2. Release read lock
3. SIGCHLD fires → OnSigChild takes write lock → CheckChildren runs
4. Child is NOT yet in s_childProcessWaitStates → SIGCHLD is consumed
without reaping the child
5. ProcessWaitState.Holder created (too late)
6. WaitForExit waits on _exitedEvent that will never be set → HANG
The fix mirrors the original Process.Unix.cs code: create the
ProcessWaitState.Holder while still holding the read lock, so the
child is in s_childProcessWaitStates before SIGCHLD can be processed.
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/325db55f-9e1f-43bd-abec-1b60c0b2b5ce
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
@tmds

tmds commented Mar 30, 2026

Copy link
Copy Markdown
Member

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@rokonec@eiriktsarpalis Could you please review the Windows part? In the first commit (968d39a) I've just moved the code to other files to allow for exposing new APIs that don't have a dependency on Process. I've added GH comments in places where I've introduced changes to Windows implementation.

@adamsitnik

Copy link
Copy Markdown
MemberAuthor

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

I've not investigated it myself (yet).

@rokonecrokonec left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@adamsitnik
adamsitnik merged commit dee9005 into dotnet:mainMar 30, 2026
93 of 97 checks passed

@eiriktsarpaliseiriktsarpalis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few minor post-merge comments:


if (!startInfo.UseShellExecute)
{
if (childInputHandle is null && !OperatingSystem.IsAndroid())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Out of curiosity, what's specific about android in this check?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Android does not support Console.OpenStandard[Input/Output/Errror]Handle methods as it does not support Console, but it still has some standard input/output/error handles, so we just pass null here and the native layer takes care of it.

return true;
}

// On Unix, we don't use process descriptors yet, so we can't get PID.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does "yet" mean in this context? Is it something we haven't implemented in the PAL? Do we need to link to an open issue?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I intend to extend SafeProcessHandle with process descriptors support for 11, but just not in this PR.

public sealed partial class SafeProcessHandle : SafeHandleZeroOrMinusOneIsInvalid
{
internal static readonly SafeProcessHandle InvalidHandle = new SafeProcessHandle();
private int _processId = -1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems like a prime use case for using the new field keyword.

: new("sh") { ArgumentList = { "-c", "exit 42" } };

using SafeProcessHandle processHandle = SafeProcessHandle.Start(startInfo);
Assert.NotEqual(0, processHandle.ProcessId);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should there be a test checking for the validation code in this property getter?

@jkotas

Copy link
Copy Markdown
Member

@jkotas@adamsitnik I wonder if you have some insights why Process ends up being difficult to trim?

It always drags in a lot of performance counter related functionality and the dependencies are not straightforward to break without observable behavior changes. (It may be possible to improve this, it is just not straightforward.)

For example, here is a dependency chain from https://github.com/MichalStrehovsky/sizoscope for a simple app that just does Process.Start("notepad.exe").WaitForExit()

image

@jkotas

Copy link
Copy Markdown
Member

Opened #126332

adamsitnik added a commit that referenced this pull request Mar 31, 2026
…te) (#126314)
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 30, 2026
Sign up for freeto 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.

Implement SafeProcessHandle.Start

7 participants

@adamsitnik@tmds@jkotas@eiriktsarpalis@rokonec