Process: use overlapped I/O for parent end of stdout/stderr pipes on Windows#125643
Conversation
…/stderr pipes Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
adamsitnik
left a comment
There was a problem hiding this comment.
Modulo comments, let's wait for CI results and get some benchmark numbers.
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
- we have to specify inBufferSize, otherwise the first write always blocks until reading starts - set the same timeout as CreatePipe (120s)
This comment was marked as resolved.
This comment was marked as resolved.
|
For a following app: Stopwatch stopwatch = Stopwatch.StartNew();
Parallel.For(0, 1_000, static (_, _) =>
{
using (Process process = new())
{
process.StartInfo = Benchmarks.s_outputStartInfo;
process.OutputDataReceived += Benchmarks.s_ignoreOutputLine;
process.Start();
process.BeginOutputReadLine();
process.WaitForExit();
process.OutputDataReceived -= Benchmarks.s_ignoreOutputLine;
}
});
Console.WriteLine($"Total time: {stopwatch.Elapsed}");The throughput doubles: Total time (before): 00:00:19.7916593
Total time (after): 00:00:09.1419538 |
There was a problem hiding this comment.
Pull request overview
Enables overlapped (async) I/O for the parent’s read end of redirected stdout/stderr pipes on Windows to avoid consuming a thread-pool thread per pipe for blocked reads, while keeping the child’s std handles synchronous for compatibility with GetStdHandle() usage.
Changes:
- Add an opt-out switch (
DOTNET_SYSTEM_DIAGNOSTICS_PROCESS_DISABLE_ASYNC_READS) to disable overlapped reads for redirected stdout/stderr. - Extend
Process.CreatePipeto control async behavior and route stdout/stderr throughSafeFileHandle.CreateAnonymousPipe(..., asyncRead: true). - Adjust the named-pipe creation parameters used by
SafeFileHandle.CreateAnonymousPipewhen async is requested (buffer sizing / timeout arguments).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs | Tweaks named-pipe handle creation parameters used when creating async-capable anonymous pipes on Windows. |
| src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs | Adds opt-out env var and uses async/overlapped reads for the parent side of redirected stdout/stderr pipes. |
You can also share your feedback on Copilot code review. Take the survey.
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
All Windows CI legs are green (also tested locally), the PR is ready for review.
@stephentoub @jkotas this is what it would take to use async IO to read from STD OUT/ERR (we don't need to set anything else thanks to #123761).
The micro benchmark results are available here. There are no regressions for ReadOutputLineByLine/ReadOutputToEndAsync, however ReadOutputToEnd is slightly slower. I've added new benchmarks to the performance repo as well (dotnet/performance#5152).
The main benefit is improved throughput (we are no longer blocking up to two thread pool threads per process with redirected std out and err). This is going to impact various tools that spawn multiple processes like test runners.
More context: #81896
Since we are in early preview I believe it's worth considering making this change (with an ability to disable it via env var).
The Unix part does not need this fix, because it's using System.IO.Pipes backed by a socket which enables the non-blocking IO with first call to any Async method.
|
@copilot please invoke the code-review skill and post your analysis/results as a comment on this PR |
|
/azp list |
This comment was marked as resolved.
This comment was marked as resolved.
|
/azp run runtime-libraries-coreclr outerloop-windows |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This is not true. I've verified that with Windows source code (any MSFT reviewers please reach out to me offline to get a link). Since Windows source code is closed source I can't post a screen shot here.. |
|
@copilot please invoke the code-review skill and post your analysis/results as a comment on this PR |
|
/azp run runtime-libraries-coreclr outerloop-windows |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Acknowledged — I was wrong about that. I'll correct the review accordingly.
🤖 Copilot Code ReviewMulti-model review (Claude Sonnet 4.6 + Gemini 2.5 Pro + GPT-4.1) Holistic AssessmentMotivation: Synchronous anonymous pipes force a blocked thread-pool thread per redirected stdout/stderr stream. Benchmark evidence (2× throughput, ~10s vs ~20s for 1,000 concurrent processes) confirms the real-world impact. Approach: Using overlapped I/O exclusively for the parent's read end of stdout/stderr pipes is the correct Windows mechanism. The child-side handles are kept synchronous by duplicating the write end (which was opened without Summary: ✅ LGTM. No blocking issues. All three models independently found no errors or correctness concerns. A few minor cosmetic suggestions below. Detailed Findings✅ Correctness: child handles remain synchronousFor stdout/stderr ( ✅ API surface
✅ AppContainer support
✅
|
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs
Show resolved
Hide resolved
|
@stephentoub @jkotas I've fixed the only edge case I was able to find (using a specific name for named pipes in container apps), the CI is green and the PR is ready for review. PTAL |
src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs
Show resolved
Hide resolved
| SafeFileHandle.CreateAnonymousPipe(out SafeFileHandle readHandle, out SafeFileHandle writeHandle); | ||
| // Only the parent's read end benefits from async I/O; stdin is always sync. | ||
| // asyncRead applies to the read handle; asyncWrite to the write handle. | ||
| SafeFileHandle.CreateAnonymousPipe(out SafeFileHandle readHandle, out SafeFileHandle writeHandle, asyncRead: !parentInputs, asyncWrite: false); |
There was a problem hiding this comment.
In few weeks from now I will be on parental leave. If this PR introduced a bug, the only thing that needs to be reverted is following line:
| SafeFileHandle.CreateAnonymousPipe(out SafeFileHandle readHandle, out SafeFileHandle writeHandle, asyncRead: !parentInputs, asyncWrite: false); | |
| SafeFileHandle.CreateAnonymousPipe(out SafeFileHandle readHandle, out SafeFileHandle writeHandle, asyncRead: false, asyncWrite: false); |
And an update for the introduced test. The other changes in this PR are bug fixes.
Anonymous pipes used for stdout/stderr redirection were always created synchronous on Windows, causing a blocked thread-pool thread per pipe read. This change opens the parent's end of stdout/stderr pipes with overlapped (async) I/O unconditionally, while keeping the child's end synchronous so that child processes using
GetStdHandle()continue to work correctly.Description
CreatePipesimplified: thebool parentInputsparameter directly drivesasyncRead: !parentInputs— stdout/stderr (parentInputs=false) unconditionally get an overlapped read handle; stdin (parentInputs=true) remains fully synchronous. No separateasyncReadsparameter is needed.FileStreamconstruction updated to usehandle.IsAsyncinstead of hardcodedfalse, so the stream's async dispatch mode matches the handle's actual overlapped mode.\\.\pipe\LOCAL\prefix, which is required for Windows AppContainers (Windows 10 v1709+). Without this prefix, pipe creation fails in UWP/AppContainer processes.CreatePipe.defaultTimeoutset to 120 seconds to match the actual internal default used byCreatePipe(verified via Windows source code).FILE_FLAG_OVERLAPPED), and why theLOCALnamespace is used.Start_Redirect_StandardHandles_UseRightAsyncMode) verifies that the parent's stdout/stderr handles are async on Windows and that the child's standard handles are always synchronous.The Unix part does not need this fix because it uses
System.IO.Pipesbacked by a socket, which enables non-blocking I/O with the first call to anyAsyncmethod.Benchmark Results
Throughput roughly doubles when many processes with redirected stdout are run concurrently:
Micro-benchmark results: EgorBot/Benchmarks#44 (comment)
Note:
ReadOutputToEnd(synchronous read on an async handle) is slightly slower due to overlapped I/O bookkeeping overhead;ReadOutputLineByLineandReadOutputToEndAsyncshow no regression.🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.