Add Process.ReadAllTextAsync and ReadAllBytesAsync

[Copilot]

Description

Adds asynchronous ReadAllTextAsync(CancellationToken) and ReadAllBytesAsync(CancellationToken) APIs to System.Diagnostics.Process, enabling fully async multiplexed reads of both stdout and stderr.

Implementation

• Async per-stream helper: ReadPipeToBufferAsync(Stream, CancellationToken) reads a single stream into a pooled buffer, owns the buffer lifecycle (returns to pool on failure via catch, transfers ownership via ArraySegment on success).
• Shared helper: ReadAllBytesIntoRentedArraysAsync validates state, launches two parallel ReadPipeToBufferAsync tasks via Task.WhenAll, handles buffer cleanup for the successful task when the other fails, and surfaces both exceptions via AggregateException when both fail.
• Sync path improvements: Refactored GetSafeFileHandleFromStreamReader into OS-specific GetSafeHandleFromStreamReader helpers (Process.Multiplexing.Windows.cs, Process.Multiplexing.Unix.cs), using SafePipeHandle on Unix and SafeFileHandle on Windows. Simplified sync ReadAllText/ReadAllBytes to use AsSpan() directly.
• Tests: Extended ProcessMultiplexingTests with useAsync parameter across all existing test methods. Added cancellation test (ReadAllAsync_ThrowsOperationCanceledOnCancellation) and multi-exception test (ReadAllAsync_ThrowsAllAvailableExceptions).

Testing

• ✅ Build verified (0 errors, 0 warnings)
• ✅ Tests verified (467 total, 0 failed, 5 skipped)
• ✅ Multi-model code review: no blocking issues found

[dotnet-policy-service]

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

[adamsitnik]

@copilot please check latest commits, do NOT touch the code, just update the PR description and then invoke the code-review skill and post your analysis/results as a comment on this PR

[Copilot]

Pull request overview

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

[Copilot]

@copilot please check latest commits, do NOT touch the code, just update the PR description and then invoke the code-review skill and post your analysis/results as a comment on this PR

PR description updated. Here is the code review:

🤖 Copilot Code Review — PR #126942

Holistic Assessment

Motivation: The changes since the merge base (8875214c) simplify the async implementation by replacing ReadPipesToBuffersAsync (a complex WhenAny-based multiplexing loop) with a simple per-stream ReadPipeToBufferAsync helper called twice in parallel. @adamsitnik's follow-up commit (442fb0f2) further improves the buffer return pattern.

Approach: The two-task Task.WhenAll pattern is simpler, has fewer allocations (eliminates per-read WhenAny task), and uses the modern ReadAsync(Memory<byte>) overload. Buffer ownership is cleanly split between the helper (owns on failure) and the caller (owns on success).

Summary: ✅ LGTM. The implementation is correct, simpler than what it replaces, and handles all buffer lifecycle edge cases properly. Verified by multi-model review (Gemini 2.5 Pro, GPT 5.3 Codex) — zero issues found across both models.

---

Detailed Findings

✅ Buffer lifecycle — Correct ownership transfer

ReadPipeToBufferAsync rents a buffer, returns it in the catch block on failure, and transfers ownership via ArraySegment on success. The callers' finally blocks use IsCompletedSuccessfully guards to return only successfully-completed buffers — preventing double-return when a task faults/cancels (since the helper's catch already returned that buffer). Flagged as correct by 2/2 models.

✅ Exception handling — Exactly-once buffer return

When one task faults and the other succeeds: the faulted task's buffer was returned in ReadPipeToBufferAsync.catch, and the successful task's buffer is returned in the caller's finally. When both fault: both helpers return their own buffers. When both succeed: caller's finally returns both. All paths result in exactly one return per buffer. Verified by 2/2 models.

✅ Cancellation — Clean propagation

CancellationToken flows through to stream.ReadAsync. On cancellation, OperationCanceledException triggers the helper's catch (buffer returned), then propagates through Task.WhenAll to the caller. Verified by 2/2 models.

✅ Performance — Reduced allocations

Eliminates per-read Task.WhenAny allocation from the old multiplexing loop. Each stream reads independently without coordination overhead. Uses ReadAsync(Memory<byte>) instead of legacy ReadAsync(byte[], int, int). Confirmed improvement by 2/2 models.

✅ No new public API surface in this diff

The ref assembly was not modified in these commits — new public APIs were added in earlier commits that are already part of the merge base.

[adamsitnik]

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

[Copilot]

Pull request overview

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

[Copilot]

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

PR description updated and code review completed. Posting results below.

[github-actions]

🤖 Copilot Code Review — PR #126942

Note

This review was AI/Copilot-generated. Multi-model review: primary analysis by Claude Opus 4.6, cross-checked by GPT-5.3-Codex and Claude Sonnet 4.5.

Holistic Assessment

Motivation: Justified. This implements @MihaZupan's suggestion to replace the Task.WhenAny multiplexing loop with two independent parallel ReadPipeToBufferAsync tasks. The old approach allocated a Task per WhenAny call on every read iteration and was unnecessarily complex for reading two independent pipes. The parallel approach is the natural design.

Approach: Correct and simpler. Each stream gets its own read loop (ReadPipeToBufferAsync), eliminating the interleaved state tracking. Buffer ownership is clean: the helper owns on failure (returns to pool in catch), transfers on success (caller returns in finally). The shared ReadAllBytesIntoRentedArraysAsync helper properly handles the "one succeeds, one fails" case by returning the successful buffer to the pool before rethrowing. The rename from GetSafeFileHandleFromStreamReader to GetSafeHandleFromStreamReader is appropriate since Unix returns SafePipeHandle, not SafeFileHandle.

Summary: ✅ LGTM. The refactoring is correct, simpler than what it replaces, and handles all buffer lifecycle edge cases properly. No blocking issues found. Two minor suggestions below.

---

Detailed Findings

✅ Buffer ownership — Correct across all code paths

Verified all paths through ReadPipeToBufferAsync and ReadAllBytesIntoRentedArraysAsync:

• Success: ReadPipeToBufferAsync returns ArraySegment → caller (ReadAllTextAsync/ReadAllBytesAsync) returns buffer in finally. ✓
• Per-task failure: ReadPipeToBufferAsync returns its buffer in catch, rethrows. ✓
• Mixed success+failure: ReadAllBytesIntoRentedArraysAsync catches, returns the successful task's buffer via IsCompletedSuccessfully check, rethrows the failure. ✓
• Both tasks fail: Neither task is IsCompletedSuccessfully, so no pool return at this level (each task already returned its own buffer in its catch). No double-return. ✓
• Cancellation: Both tasks cancel via shared token → both ReadPipeToBufferAsync catch blocks return buffers → whenAll is Canceled (not Faulted) → whenAll.Exception is null → throw; rethrows OperationCanceledException. ✓

✅ Exception handling — Correct for all scenarios

• Both tasks fault (non-cancellation): whenAll.Exception.InnerExceptions.Count > 1 → throws AggregateException with both exceptions. Tested by ReadAllAsync_ThrowsAllAvailableExceptions(multiple: true). ✓
• One task faults: Single inner exception → throw; rethrows the original unwrapped exception. Tested by ReadAllAsync_ThrowsAllAvailableExceptions(multiple: false). ✓
• Both tasks cancel: Tasks transition to Canceled state (not Faulted), so whenAll.Exception is null → throw; rethrows TaskCanceledException. Tested by ReadAllAsync_ThrowsOperationCanceledOnCancellation. ✓

Note: Claude Sonnet 4.5 flagged a potential cancellation bug where both tasks being cancelled would throw AggregateException instead of OperationCanceledException. This is a false positive: when an async method throws OperationCanceledException, the AsyncTaskMethodBuilder calls TrySetCanceled, putting the Task in Canceled state (not Faulted). Task.WhenAll on all-Canceled tasks produces a Canceled task whose Exception property is null, so whenAll.Exception?.InnerExceptions.Count > 1 evaluates to false and throw; correctly rethrows the TaskCanceledException.

✅ Modern API usage — Good improvement

ReadPipeToBufferAsync uses stream.ReadAsync(buffer.AsMemory(bytesRead), cancellationToken) (the ValueTask<int>-returning overload) instead of the old stream.ReadAsync(byte[], int, int, CancellationToken) (the Task<int>-returning overload). This avoids a Task<int> allocation when the read completes synchronously.

✅ Test coverage — Adequate

The new ReadAllAsync_ThrowsAllAvailableExceptions test covers both the "both fail → AggregateException" and "one fails → single exception" scenarios with all four combinations of (multiple, bytes). The use of reflection to access _standardOutput/_standardError is pragmatic — the comment explains why (accessing the properties would set StreamReadMode, preventing ReadAllBytesAsync from being called). The existing cancellation test is preserved unchanged.

💡 throw whenAll.Exception stack trace — Non-issue, just noting

Process.Multiplexing.cs line 219: throw whenAll.Exception; creates a new stack trace on the AggregateException wrapper. This is fine — the important stack traces (in the inner exceptions) are fully preserved. Since Task.WhenAll constructs the AggregateException internally without ever throwing it, there's no prior stack trace to preserve. ExceptionDispatchInfo.Throw would not help here.

Flagged by GPT-5.3-Codex as ⚠️; assessed as non-issue after deeper analysis.

💡 Test suggestion: mixed-outcome with stderr (follow-up, not blocking)

The current ReadAllAsync_ThrowsAllAvailableExceptions(multiple: false) covers the "stdout disposed, stderr succeeds" case. A variant with stderr disposed (and stdout succeeds) would exercise the mirror path through the IsCompletedSuccessfully checks. Low priority — the code is symmetric and already tested.

Generated by Code Review for issue #126942 · ◷