Skip to content

Implement Process.ReadAllText and ReadAllBytes with platform-specific multiplexing#126807

Merged
adamsitnik merged 20 commits intomainfrom
copilot/implement-readalltext-and-readallbytes
Apr 15, 2026
Merged

Implement Process.ReadAllText and ReadAllBytes with platform-specific multiplexing#126807
adamsitnik merged 20 commits intomainfrom
copilot/implement-readalltext-and-readallbytes

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 12, 2026

Description

Implements Process.ReadAllText and Process.ReadAllBytes — new APIs (approved in #125838) that drain both stdout and stderr without deadlocking, using single-threaded synchronous multiplexing. Both standard output and standard error must be redirected; the methods throw InvalidOperationException if either is missing. The methods also require that neither StandardOutput nor StandardError has been accessed in sync mode (StreamReadMode.Undefined required), preventing silent data loss from StreamReader-buffered data.

using Process process = Process.Start(startInfo);
(string stdout, string stderr) = process.ReadAllText(timeout: TimeSpan.FromSeconds(30));
(byte[] stdoutBytes, byte[] stderrBytes) = process.ReadAllBytes();

Windows — Overlapped IO

  • ReadPipes is private static unsafe void with all resources (overlapped, events) declared at method top and cleaned up in a single finally block — ensures no leaks if any allocation throws
  • Uses PinnedGCHandle<byte[]> for buffer pinning — lower overhead than MemoryHandle (it's what Memory.Pin uses underneath), and allows reassigning Target when the buffer grows without needing to dispose and recreate the pin
  • NativeOverlapped* with ManualResetEvent per pipe
  • ReadFile (overlapped) → WaitHandle.WaitAnyGetOverlappedResult loop
  • CancelIoEx on timeout, GetOverlappedResult(bWait: true) to observe completion before freeing OVERLAPPED — references official MSDN docs for the GetOverlappedResult contract
  • AllocateOverlapped uses NativeMemory.AllocZeroed and only sets the meaningful EventHandle field — no redundant zero assignments
  • WaitHandle array orders stderr before stdout so that when both are signaled, WaitAny services error output first — important for timeout scenarios with heavy output, and for the upcoming line-streaming PR
  • Uses is not null pattern matching syntax throughout

Unix — Non-blocking IO with poll(2)

  • Pipes are set to non-blocking mode at the start using Interop.Sys.Fcntl.DangerousSetIsNonBlocking via raw file descriptors — avoids issues with AnonymousPipeClientStream/Socket internal blocking/non-blocking state management
  • Interop.Sys.Poll on stdout/stderr file descriptors for readiness notification
  • ReadNonBlocking helper method uses Interop.Sys.Read (direct syscall) instead of RandomAccess.Read — prevents the framework from calling poll internally on blocking reads, which would not respect our timeout. EAGAIN is handled gracefully (returns 0 bytes read)
  • Simplified poll event check: == POLLNONE instead of masking individual flags
  • EINTR handling with deadline recalculation
  • Poll errors translated via Interop.Sys.ConvertErrorPalToPlatform(error) for proper errno in Win32Exception
  • ReadPipes is a regular private static void (non-nullable SafeFileHandle parameters)

Common (Process.Multiplexing.cs)

  • ValidateReadAllState: validates state (disposed, both streams redirected, async mode conflict, sync mode conflict) and sets sync mode — called from ReadAllText and ReadAllBytes before renting arrays
  • ReadPipesToBuffers: extracts file handles via GetSafeFileHandleFromStreamReader, calls DangerousAddRef/DangerousRelease to protect handle lifetime, delegates to platform-specific ReadPipes
  • GetSafeFileHandleFromStreamReader: returns SafeFileHandle + out SafeHandle owner for both FileStream and AnonymousPipeClientStream base streams; throws UnreachableException for unknown types
  • RentLargerBuffer: doubles buffer via ArrayPool with Array.MaxLength cap using Math.Max(buffer.Length + 1, newSize) to guarantee forward progress; lets ArrayPool throw OutOfMemoryException with its descriptive "Array dimensions exceeded supported range" message instead of throwing manually. Swaps buffer assignment before returning old buffer to pool to prevent any path to ArrayPool double-free.
  • Timeout conversion uses ToTimeoutMilliseconds() for safe range-checked conversion

Test coverage (25 test cases)

  • Validation: disposed process, missing stdout/stderr redirects, async mode conflict, sync mode conflict
  • Success: reads both stdout and stderr correctly for text and bytes
  • Large output: 1MB+ data to exercise buffer growth
  • Timeout: TimeoutException when child process blocks
  • Empty output: both streams produce empty results
  • Interleaved output: rapid alternation between stdout/stderr (100 iterations) with exact string equality verification
  • Binary data with null bytes: verifies ReadAllBytes handles embedded null bytes correctly

Copilot AI and others added 4 commits April 12, 2026 07:31
…le fixes needed)

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/45ece5ba-166f-4418-887f-3ecaba54615e

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
…ead of GetAwaiter().GetResult()

Replace the async Task-based Windows implementation with proper Win32 overlapped IO:
- Allocate NativeOverlapped* with ManualResetEvent for each pipe
- Use Interop.Kernel32.ReadFile with overlapped structs
- Use WaitHandle.WaitAny for synchronous multiplexing
- Use GetOverlappedResult to retrieve bytes read
- Use CancelIoEx for timeout cancellation
- Add System.Threading.Overlapped project reference
- Add ReadFile, GetOverlappedResult, CancelIoEx interop references
- Fix nullable warning in Unix implementation

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/8af96aef-e52b-459d-a945-e1159e8be19b

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
…in32Exception

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/8af96aef-e52b-459d-a945-e1159e8be19b

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 12, 2026 09:31
Copilot AI review requested due to automatic review settings April 12, 2026 09:31
@adamsitnik adamsitnik added the NO-REVIEW Experimental/testing PR, do NOT review it label Apr 12, 2026
Copy link
Copy Markdown
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

@copilot please address my feedback

…elper method, DangerousAddRef/Release, remove partial

Changes:
1. Require both stdout and stderr to be redirected (throw if either is missing)
2. Create ReadPipesToBuffers helper shared between ReadAllText and ReadAllBytes
   (ReadAllText avoids extra buffer copy by decoding from rented buffers directly)
3. GetSafeFileHandleFromStreamReader now returns an owner SafeHandle; caller uses
   DangerousAddRef/DangerousRelease to prevent handle disposal during reads
4. Remove partial method declaration; ReadPipes is a regular static method in each
   platform file (non-nullable parameters since both handles always provided)
5. Update tests: all tests redirect both stdout and stderr; add two new tests for
   missing-stream validation

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/cd917b13-f344-47c5-ac1e-cfb2c2da34f6

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot April 12, 2026 10:44
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 12, 2026

@copilot please address my feedback

All feedback addressed in 0986690. Summary of changes:

  1. Both stdout and stderr must be redirected (strict validation with separate error messages)
  2. ReadPipesToBuffers helper shared between ReadAllText and ReadAllBytes (avoids extra buffer copy in ReadAllText)
  3. GetSafeFileHandleFromStreamReader returns out SafeHandle owner; caller uses DangerousAddRef/DangerousRelease
  4. Removed partial method declaration — ReadPipes is a regular static method per platform with non-nullable parameters
  5. All tests updated to redirect both streams; added 2 new validation tests (17/17 pass)

Copilot AI requested a review from adamsitnik April 12, 2026 10:46
Copy link
Copy Markdown
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

@copilot address my feedback

…indows ReadPipes fully unsafe with proper cleanup, use is not null

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/451cf437-2de0-409a-937a-0031c3fa69d4

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot April 12, 2026 11:36
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 12, 2026

@copilot address my feedback

All feedback addressed in df0e65aValidateReadAllState helper extracted, Windows ReadPipes made fully unsafe with proper cleanup ordering, is not null syntax. 17/17 tests pass.

Copilot AI requested a review from adamsitnik April 12, 2026 11:38
- refactor the code
- restore deleted comments
- fix AV bug by setting the lower bit for EventHandle
- fix test bug: process argument can't be that long
- refactor the tests to use Theories rather than very similar Fact for bytes and text
Copilot AI requested review from Copilot and removed request for Copilot April 14, 2026 19:02
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 14, 2026

Just a couple comments on the Windows code, which looks pretty good to me.

Both items addressed in 6ecc0b9:

  1. WaitHandle array flipped to [errorEvent, outputEvent] so stderr is prioritized when both are signaled
  2. Raymond Chen blog post links added as XML doc on CancelPendingIOIfNeeded

…ly official docs references

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ef4fa489-ab10-4574-9069-ca32ccaac99b

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot April 14, 2026 19:13
Copy link
Copy Markdown
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

@copilot address my feedback

…Read with EAGAIN handling

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/2646b5c6-b0ae-4ae3-99ca-064187661e73

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot April 14, 2026 20:36
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 14, 2026

@copilot address my feedback

Both items addressed in c92f673:

  1. Non-blocking mode set via Interop.Sys.Fcntl.DangerousSetIsNonBlocking with raw FDs
  2. RandomAccess.Read replaced with Interop.Sys.Read in a ReadNonBlocking method that handles EAGAIN

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

🤖 Copilot Code Review — PR #126807

Note

This review was AI/Copilot-generated using multi-model analysis (Gemini 2.5 Pro, GPT-5.3-Codex).

Holistic Assessment

Motivation: Implements approved API surface (ReadAllText/ReadAllBytes) from #125838 that solves the well-documented deadlock problem when reading both stdout and stderr.

Approach: Platform-specific multiplexing (overlapped IO on Windows, poll(2) with non-blocking reads on Unix) is the correct approach for single-threaded synchronous draining of two pipes.

Summary: ✅ LGTM. All previous review feedback from @adamsitnik, @jkotas, @tmds, and @JeremyKuhne has been addressed. The implementation is solid with proper resource management, handle lifetime protection, and comprehensive test coverage (25 test cases).


Detailed Findings

✅ API shape matches approved proposal

The ref assembly signatures exactly match the approved shape from @bartonjs's API review comment on #125838. Tuple element names are PascalCase (StandardOutput, StandardError). ReadAllBytes precedes ReadAllText alphabetically, both before Refresh().

✅ Non-blocking IO approach is correct (Unix)

DangerousSetIsNonBlocking with raw FDs correctly sets O_NONBLOCK before entering the poll loop. ReadNonBlocking uses Interop.Sys.Read and correctly distinguishes: positive (data), zero (EOF), negative-with-EAGAIN (spurious wakeup → retry poll). POLLHUP-only wakeups are handled correctly because read() returns 0 on a HUP'd pipe.

✅ Resource management is correct (Windows)

PinnedGCHandle<byte[]> replaces previous MemoryHandle approach — less overhead, and Target can be reassigned on buffer growth without dispose/recreate. All resources (pins, overlapped structs, events) are properly cleaned up in the finally block. CancelPendingIOIfNeeded correctly calls GetOverlappedResult(bWait: true) to observe completion before the OVERLAPPED is freed.

✅ Buffer growth and OOM protection

RentLargerBuffer uses Math.Max(buffer.Length + 1, newSize) to guarantee forward progress even at Array.MaxLength, and lets ArrayPool throw the descriptive "Array dimensions exceeded supported range" OOM. Swap-then-return ordering prevents ArrayPool double-free.

✅ Handle lifetime protection

DangerousAddRef/DangerousRelease properly bracket the ReadPipes call in ReadPipesToBuffers, preventing handle recycling during native I/O operations.

✅ Encoding fallback is consistent with existing code

GetStandardOutputEncoding() is used as fallback for both stdout and stderr. This matches the existing pattern in Process.cs lines 1307-1312 where the same method is used for both streams.

✅ Test coverage is comprehensive

25 test cases cover: validation (disposed, missing redirects, async conflict, sync conflict), success paths (text and bytes), large output (buffer growth), timeout, empty output, interleaved output (100 iterations with exact equality), and binary data with null bytes.

@adamsitnik adamsitnik requested review from JeremyKuhne and tmds April 14, 2026 20:45
Copilot AI requested a review from adamsitnik April 14, 2026 20:47
}

_outputStreamReadMode = StreamReadMode.SyncMode;
_errorStreamReadMode = StreamReadMode.SyncMode;
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.

These modes could use more descriptive names.

We should decide if this doesn't require a separate mode.

A mode is meant to prevent the user from mixing different APIs that shouldn't be used together.

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.

I will experiment with that idea locally and send a follow up PR if I get to the point I like it.

Copy link
Copy Markdown
Member

@tmds tmds left a comment

Choose a reason for hiding this comment

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

lgtm, only open question is if this shouldn't be a separate read mode.

/// <summary>
/// Rents a larger buffer from the array pool and copies the existing data to it.
/// </summary>
private static void RentLargerBuffer(ref byte[] buffer, int bytesRead)
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.

If you wanted to avoid needing such buffer management logic, you could consider pulling in the ArrayBuffer helper from the Common folder to simplify all this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement Process.ReadAllText and Process.ReadAllBytes methods

8 participants