Skip to content

fix: StreamedBinaryFileResponse — Incorrect Chunking Logic#121

Merged
s2x merged 2 commits intomasterfrom
fix/streamed-binary-file-response-chunking
Apr 12, 2026
Merged

fix: StreamedBinaryFileResponse — Incorrect Chunking Logic#121
s2x merged 2 commits intomasterfrom
fix/streamed-binary-file-response-chunking

Conversation

@s2x
Copy link
Copy Markdown
Collaborator

@s2x s2x commented Apr 12, 2026

Summary

Fixes incorrect chunking logic in StreamedBinaryFileResponse by simplifying the inner while loop that was unnecessarily complex and had a bug where $length was decremented by $read instead of actual data length.

Changes

  • Simplified the chunking logic in src/Protocol/Http/Response/StreamedBinaryFileResponse.php:36-52
  • Removed redundant inner while ('' !== $data) loop that only executed once in most cases
  • Fixed length calculation: uses strlen($data) instead of $read because fread() may return fewer bytes than requested
  • Added explicit check for empty string ($data === '') alongside the existing false check for more robust handling
  • Changed break 2 to single break since we're now only using one loop level

Why

The original code had two issues:

  1. The inner loop was effectively a no-op - substr($data, $read) when $read >= strlen($data) returns empty string, causing the loop to execute exactly once
  2. $length -= $read was incorrect - should use actual data length since fread() can return fewer bytes than requested

Testing

  • All 261 existing tests pass
  • Static analysis (phpstan, php-cs-fixer, rector) passes

Closes #27

@s2x
Copy link
Copy Markdown
Collaborator Author

s2x commented Apr 12, 2026

Review of this PR completed via coder-model (Qwen Code /review):

This PR is a clean, correct simplification of the chunking logic in StreamedBinaryFileResponse::streamContent().

Deterministic checks: PHPStan ✅ | php-cs-fixer ✅ | Rector ✅ | PHPUnit (261 tests) ✅

Why each change is correct:

  • Removing the inner while ('' !== \$data) loop — in Symfony's BinaryFileResponse::sendContent() this retries partial fwrite calls, but in a generator model yield delegates I/O to the consumer, so the retry loop is unnecessary
  • \$length -= strlen(\$data) fixes the original bug where \$length -= \$read used the requested byte count instead of actual data length from fread()
  • \$data === '' check alongside false is sound defensive handling
  • break instead of break 2 is correct after removing the nested loop

LGTM ✅ Ready to merge.

@s2x s2x merged commit 8098175 into master Apr 12, 2026
21 checks passed
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.

StreamedBinaryFileResponse — Incorrect Chunking Logic

1 participant