Skip to content

Use double buffering when downloading with progress - #129

Open
lostmsu wants to merge 1 commit into
CmlLib:masterfrom
BorgGames:download-cancel
Open

Use double buffering when downloading with progress#129
lostmsu wants to merge 1 commit into
CmlLib:masterfrom
BorgGames:download-cancel

Conversation

@lostmsu

@lostmsu lostmsu commented Jan 18, 2026

Copy link
Copy Markdown

Should speed it up a little.

@AlphaBs

AlphaBs commented Jul 28, 2026

Copy link
Copy Markdown
Member

Thanks for the contribution. I tested this change and found two issues that should be addressed before merging:

  1. 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.

  2. 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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants