You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Thanks for the contribution. I tested this change and found two issues that should be addressed before merging:
Task.WhenAll(writing, reading) can hide an I/O failure behind the other pending operation. For example, if WriteAsync fails immediately because the disk is full while the concurrent network ReadAsync stalls, the method does not surface the write error until the read completes. Neither operation receives the cancellation token, so cancellation cannot release the current wait. I reproduced this with a failing destination stream and a blocked source stream: the download remained pending until I manually completed the read. Please use the cancellation-aware read/write overloads and cancel and observe the peer operation when either side fails, or avoid this pipelining approach.
Progress is reported for the newly read buffer before that buffer is written to the destination. With a one-chunk, 4-byte response, the 100% progress callback observed a destination length of 0. If cancellation or a progress callback exception occurs at that point, the last buffer is never written even though 100% was reported. Please track written bytes and report progress only after the corresponding WriteAsync has completed.
There is also an unrelated cancellation behavior change: replacing the previous early return with ThrowIfCancellationRequested changes mid-download cancellation from successful completion to OperationCanceledException, and only for the progress-enabled path. This may be the preferable contract, but it should be documented and tested, and the no-progress CopyToAsync path should behave consistently.
The existing test project passes locally (159 passed, 4 platform-specific tests skipped), but there are currently no tests covering this helper. Regression tests for short reads, read/write failures, cancellation, and progress ordering would make this change much safer.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Should speed it up a little.