Repository navigation
Pin a byte source's expectedLength as an exact total - #7786
Conversation
|
✅ Approved — The The decision is advisory: this comment never approves, blocks or requests changes on the pull request. ✅ All checks passed🔒 Security: ✅ LGTMNo security concerns. 📖 AGENTS.md: ✅ LGTMThe 📋 Code Quality: ✅ LGTM
⚡ Performance: ✅ LGTMMoving shared code into a private helper has no performance impact. 📝 Documentation: ✅ LGTMThe comments describe the code as it is now. Reviewed commit: |
|
Since last review: 0 resolved, 1 still open, 0 new. The BYOB response refactor and expected-length coverage remain, but the prior in-range Not re-run: tests, api-compat, docs, compat-flags, design-simplicity (no author changes in their files since the last review) Reviewed commit: a5970ca5 · github run |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
eae1984 to
b9295c2
Compare
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
LGTM. The #respondInternal extraction keeps the steps in the same order, and it splits where the spec splits Respond/RespondWithNewView from RespondInternal. Verified locally: the readable-byte suite passes (22 targets), and dropping #accountDelivery from #respondInternal / #respondToReleasedHead fails only byteSourceExpectedLengthContract / byteSourceExpectedLengthHeldRequest respectively.
b9295c2 to
cb6b949
Compare
DUP-23 step 1, test only. Under TypeScript a byte source that declares expectedLength must deliver exactly that many bytes through enqueue(), respond() and respondWithNewView() combined: the delivery that goes past the total throws RangeError and errors the stream, and so does a close() short of it. No test reached either error, so the accounting on any of those paths could go without a test failing. byteSourceExpectedLengthContract pins an overflow through each of the three, the exact total followed by close(), and a close() short of the total. C++ enforces neither error: every delivery reaches the reader, and close() closes the stream. Readable-byte ledger #42 records the divergence.
DUP-23 step 2. respond() and respondWithNewView() ended with the same steps once they had validated the response and transferred the head pull-into's buffer: invalidate the byobRequest, then answer the released head, commit the pending pull-intos when the stream is closed, or count the bytes toward expectedLength, fill the head, and check for close and pull. The two copies differed only in where the byte count came from. #respondInternal, named for the spec's ReadableByteStreamControllerRespondInternal, now holds those steps. Each caller keeps its own validation and buffer transfer. Nothing observable changes. Dropping the accounting call from #respondInternal fails only byteSourceExpectedLengthContract, the step 1 pin.
DUP-23 step 3, test only. When a reader releases a pending read(view) whose byobRequest the source holds, and the stream is then teed, the request survives the fork. TypeScript answers it on a path of its own, which counts the responded bytes toward expectedLength separately. No test reached that count: every response in byteSourceExpectedLengthContract answers a reader's own read. byteSourceExpectedLengthHeldRequest pins an overflow through respond() and respondWithNewView() on such a request, which errors both branches under TypeScript and reaches both under C++, and the exact total followed by close(), which closes both branches in both. Dropping the count from that path fails only this test. The overflow error and the two responses are now shared with byteSourceExpectedLengthContract. Readable-byte ledger #42 covers the held request and records TypeScript's errors as by design.
DUP-23 step 5, test only. byteSourceExpectedLengthHeldRequest pinned an overflow and the exact total through a byobRequest held across tee(), but not a close() short of the total, which byteSourceExpectedLengthContract pins on a reader's own read. It now also responds with two bytes of four and closes: under TypeScript close() throws RangeError and both branches error with it, and under C++ both branches read the two bytes and close. The close() check is the one the Contract case reaches. The underflow error is now shared by both tests, as the overflow error is.
cb6b949 to
a5970ca
Compare
2 participants
Expand test for expectedLength, deduplicate some more