Reassemble WebSocket frames into a single buffer - #13488
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #13488 +/- ##
==========================================
- Coverage 99.00% 99.00% -0.01%
==========================================
Files 132 132
Lines 49635 49724 +89
Branches 2575 2584 +9
==========================================
+ Hits 49141 49229 +88
- Misses 370 371 +1
Partials 124 124
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
|
|
Merging this PR will not alter performance
Comparing Footnotes
|
|
|
|
|
Confidence Score: 5/5The PR appears safe to merge. The previously reported repeated-prefix-copying issue no longer remains: folding joins only newly accumulated fragments, incrementally extends the bytearray, and detaches completed payload storage before the next frame. Reviews (4): Last reviewed commit: "Skip the empty fragment from a header-on..." | Re-trigger Greptile |
|
|
|
|
Confirmed independently. Rebuilt Hot path untouched. Single-read frame keeps 314 tests pass with extension. 81 pure Python. Also probed masked, unmasked, PMCE-compressed, split CLOSE, split PING, zero-length, two-frame-torn — 1/3/7-byte reads. All payloads intact. |
|
|
Measured on this branch. No codspeed regression. Cython build, best-of-N, 1 MiB payload — PR vs master: 4096-byte reads 0.27 / 0.28 ms; 64-byte reads 3.54 / 3.80 ms; 2-byte reads 100.8 / 119.4 ms. Pure Python, 2-byte reads: 316 / 400 ms. Equal or faster everywhere. Whole-message path stays byte-identical to master — On backup: mid-frame accumulation capped by |
|
|
|
|
|
|
|
|
|
The suggestion stands. When Worth adding a small pinning test since the guard is a single conditional expression — a "tidy up" refactor could flip it silently: def test_max_msg_size_zero_disables_fold() -> None:
out = mock.Mock()
out.feed_data = mock.Mock()
parser = WebSocketReader(out, 0, compress=False, decode_text=False)
assert parser._max_fragments == 0 # no fold when limits are opted outIf you want to also assert the runtime behavior, feed a small masked frame one byte at a time and check |
|
|
|
The root cause and fix are clear. Here's the one-line correction using option 1 (feed header + first payload byte together, which is also more realistic): The header-only Feeding the header and first payload byte together eliminates the empty-slice artifact and leaves the assertion correct at |
|
|
|
Now I have the full picture. The Root cause of the off-by-one: when the header is fed alone via At After That said, the cleaner fix is one line in if self._payload_bytes_to_read != 0:
if f_start_pos < f_end_pos:
self._payload_fragments.append(data_cstr[f_start_pos:f_end_pos])This eliminates the spurious empty fragment on every header-only read, avoids a needless list entry on the hot path regardless of The |
|
The root cause Kōan identifies is accurate: when For The cleaner fix is in This is one guarding line, touches only the fragment-list management, and makes the count invariant exact: The |
|
The root cause is correct. When The cleanest one-line fix is to not let the header feed reach parser.feed_data(PACK_LEN2(0x80 | WSMsgType.BINARY, 126, payload_len) + b"x")
for _ in range(payload_len - 2):
parser.feed_data(b"x")That keeps the loop structure identical, eliminates the phantom empty fragment, and means after The same header-only feed pattern affects |
|
The root cause is clear. In The final data is still correct ( There are two ways to fix it: Option A — one-line test fix (what you called out): Option B — reader fix (skip empty slices) — slightly more invasive but fixes the root cause so the count reflects "one fragment per non-empty read": In if self._payload_bytes_to_read != 0:
if f_end_pos > f_start_pos:
self._payload_fragments.append(data_cstr[f_start_pos:f_end_pos])
if (
self._max_fragments
and len(self._payload_fragments) > self._max_fragments
):
self._payload_buffer += b"".join(self._payload_fragments)
self._payload_fragments.clear()
breakOption B also needs the same change in the Cython path to stay in sync. Given the review calls it "test-only, one-line fix", Option A unblocks CI with minimal churn. Option B is cleaner semantically — the fragment count then means "number of non-empty reads" — but touches two files and requires a Cython rebuild. |
PR Review — Reassemble WebSocket frames into a single bufferMerge-ready. Both findings from my previous pass are resolved at What is specifically strong here:
Verification this pass (fresh worktree at Non-blocking:
Checklist
ℹ️ Triage summary1 pre-existing finding(s) on unchanged code suppressed (freeze). Important The branch moved during review. This review was performed against Automated review by Kōan (Claude) |
|
The Koan review is thorough and I agree with its assessment. A few observations from reviewing the diff directly: The pre-existing "empty fragment" issue Koan flagged is already fixed in this PR. The guard The unchecked checklist item ("Retained object count bounded by the fold cap") looks like an oversight — the fold at Two minor notes:
Overall the core correctness properties are well-established: buffer detach prevents aliasing, object count stays bounded at the fold cap, the unresumable |
What do these changes do?
While reassembling a WebSocket frame that arrives across many small transport reads, the reader kept one
bytesobject per read with no bound on the count. The reads are now collected in a list and joined once when the frame completes; if a frame dribbles in across more thanmax(1024, max_msg_size // 256)reads, the pending reads are folded into a singlebytearrayand the list is cleared, so the retained object count stays bounded. The common few-reads path is byte-identical to before, so large messages keep their throughput; the fold only fires for pathologically small reads and stays linear.Are there changes in behavior for the user?
No functional change; a frame split across many reads is assembled the same way, with the per-read object overhead now bounded.
Is it a substantial burden for the maintainers to support this?
No; it replaces the earlier count-cap plus
pause_reading()with a fold that keeps reading.Related issue number
Follow up to #13352
Checklist
CONTRIBUTORS.txtCHANGES/folder<issue_or_pr_num>.<type>.rst(e.g.588.bugfix.rst)Drafted with Claude Code; reviewed by bdraco.