Repository navigation
Encode string chunks as UTF-8 when pumping TypeScript streams - #8053
Conversation
|
💬 Approved with comments — The fix is correct and matches Findings: 1 suggestion The decision is advisory: this comment never approves, blocks or requests changes on the pull request. 📝 Documentation: 💬 1 suggestionOne optional clarification about the JS-to-kj bridge's byte-only wording Recommendation: 💬 Optional Nits
View full review
✅ All other checks passed🔒 Security: ✅ LGTMLone surrogates are replaced with U+FFFD, so the pump always writes valid UTF-8. The byte array keeps its backing string alive. 📖 AGENTS.md: ✅ LGTMThe AGENTS.md additions in the test suites correctly describe the new string-chunk tests. 📋 Code Quality: ✅ LGTMThe ⚡ Performance: ✅ LGTMThe string chunk path no longer makes an extra copy. The encoded buffer is taken over as it is. Reviewed commit: |
|
Since last review: 0 resolved, 0 still open, 0 new. Reviewed commit: 40beacd4 · github run |
02f8c22 to
e38d9d2
Compare
ry
left a comment
There was a problem hiding this comment.
LGTM. Matches valueToBytes() exactly.
Follow-up question: other non-byte chunks (e.g. a number) still fail the request-body pump without rejecting fetch(), leaving the body cut short. Does C++ behave the same? If not, worth an issue.
Under typescript_implemented_streams, the C++ bridge's pump for a queued (JS-sourced) stream rejected string chunks with "This ReadableStream did not return bytes." The C++ implementation's pump has always written them as their UTF-8 encoding (valueToBytes() in queue.c++), and code such as server-sent-event handlers that enqueue strings without a TextEncoder relies on it. Every sink that sends a body on goes through this pump: a fetch() request body, iterable request bodies included, a Response the fetch handler returns, cache.put(), KV put(), HTMLRewriter's input and streamed replacement content, EventSource.from() and JS RPC. As the request body pump runs apart from the response, its failure did not reject fetch(): the request went out with its body cut short at the batch holding the string, and only the source's cancel() saw the error. Elsewhere the client got an empty reply, or an RPC receiver a premature disconnect. The pump now writes a string chunk with toUSVString(), exactly as valueToBytes() does: lone surrogates become U+FFFD and embedded NULs are kept. Other non-byte chunks still reject. Reading a body in JavaScript (text() and the like) still rejects strings in both implementations, as the spec has it. Tests pin the parity in each sink's suite: the readable suite (request, iterable and returned response bodies), htmlrewriter, cache, the cross-implementation RPC test, and EventSource, which gains a TypeScript-streams config. The readable suite's main cells pin fetch_iterable_type_support for the iterable body. The bridge unit test that pinned the rejection with a string now uses a number. typescript_implemented_streams is still experimental, with no enable date, so the change needs no compatibility flag. Fixes: #8041
e38d9d2 to
40beacd
Compare
Under typescript_implemented_streams, the C++ bridge's pump for a queued (JS-sourced) stream rejected string chunks with "This ReadableStream did not return bytes." The C++ implementation's pump has always written them as their UTF-8 encoding (valueToBytes() in queue.c++), and code such as server-sent-event handlers that enqueue strings without a TextEncoder relies on it.
Every sink that sends a body on goes through this pump: a fetch() request body, iterable request bodies included, a Response the fetch handler returns, cache.put(), KV put(), HTMLRewriter's input and streamed replacement content, EventSource.from() and JS RPC. As the request body pump runs apart from the response, its failure did not reject fetch(): the request went out with its body cut short at the batch holding the string, and only the source's cancel() saw the error. Elsewhere the client got an empty reply, or an RPC receiver a premature disconnect.
The pump now writes a string chunk with toUSVString(), exactly as valueToBytes() does: lone surrogates become U+FFFD and embedded NULs are kept. Other non-byte chunks still reject. Reading a body in JavaScript (text() and the like) still rejects strings in both implementations, as the spec has it.
Tests pin the parity in each sink's suite: the readable suite (request, iterable and returned response bodies), htmlrewriter, cache, the cross-implementation RPC test, and EventSource, which gains a TypeScript-streams config. The readable suite's main cells pin fetch_iterable_type_support for the iterable body. The bridge unit test that pinned the rejection with a string now uses a number.
typescript_implemented_streams is still experimental, with no enable date, so the change needs no compatibility flag.
Fixes: #8041