Repository navigation
Pin shared-memory writes to IdentityTransformStream - #7712
Merged
Merged
Conversation
jasnell
added this pull request to stack #7710
October 13, 2026 00:17
Contributor
|
Since last review: 0 resolved, 0 still open, 0 new. Not re-run: tests, docs, api-compat, compat-flags, design-simplicity (no author changes in their files since the last review) Reviewed commit: 80c0ea48 · github run |
guybedford
approved these changes
Oct 13, 2026
guybedford
left a comment
guybedford
left a comment
Contributor
There was a problem hiding this comment.
Verified locally: all identity and compression test variants pass uncached. Checked that utils.isArrayBuffer is false for a SharedArrayBuffer so the SAB branch of bufferSourceExtent is reachable, and that a detached ArrayBuffer reads byteLength 0 in both implementations (ab.size() in C++ processChunk), so closing ledger #12 is correct.
One optional nit: the SAB branch allocates a Uint8Array just to read the length, and compression now reaches it only to throw. A SharedArrayBufferPrototypeByteLengthGet primordial would avoid that; fine as a follow-up.
jasnell
force-pushed
the
jasnell/ts-streams-duplicate-consolidaton-4
branch
from
October 13, 2026 16:58
7935867 to
b0f3feb
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7712 +/- ##
=======================================
Coverage 41.04% 41.04%
=======================================
Files 986 986
Lines 284720 284720
Branches 27618 27618
=======================================
Hits 116877 116877
Misses 151757 151757
Partials 16086 16086 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jasnell
force-pushed
the
jasnell/ts-streams-duplicate-consolidaton-4
branch
from
October 14, 2026 15:48
b0f3feb to
647f4ad
Compare
Base automatically changed from
jasnell/ts-streams-duplicate-consolidaton-3
to
main
October 14, 2026 16:24
jasnell
force-pushed
the
jasnell/ts-streams-duplicate-consolidaton-4
branch
from
October 14, 2026 16:24
647f4ad to
8b44c30
Compare
IdentityTransformStream accepts shared memory in both implementations, but no identity test wrote any. Pin it ahead of reading every BufferSource's extent through one helper: a SharedArrayBuffer, and a typed array or DataView over part of one, are each delivered as a copy taken at write(), and an empty SharedArrayBuffer is a zero-length no-op. C++ copies them in processChunk; TypeScript in validateAndCopyChunk.
The TypeScript IdentityTransformStream read an ArrayBuffer chunk's length by constructing a Uint8Array over it, which throws for a detached buffer, so the write rejected with a TypeError. Everything else treats such a buffer as empty: C++ skips any BufferSource of size 0 in processChunk; the TypeScript identity stream already did so for a detached view; CompressionStream does so for a detached ArrayBuffer in both implementations; and Web IDL's "get a copy of the bytes held by the buffer source" returns no bytes for one. Read an ArrayBuffer's length through the captured byteLength getter, which reports 0 once the buffer is detached, so the write is a zero-length no-op. A SharedArrayBuffer, which that getter rejects and which cannot be detached, keeps the read through a Uint8Array. FixedLengthStream shares the path. Flip alreadyDetachedBufferAtWrite (ledger #12) to parity, adding a FixedLengthStream(0) case. typescript_implemented_streams is experimental, so this needs no compatibility flag.
IdentityTransformStream and CompressionStream each dispatched a written chunk over ArrayBuffer, SharedArrayBuffer and views to find its bytes, and the copies drifted apart: a detached ArrayBuffer was empty in one and, until the previous commit, a TypeError in the other. Add bufferSourceExtent to view-extent.ts: the extent of a view (through viewByteExtent), or of a whole ArrayBuffer or SharedArrayBuffer, and undefined for anything else. Both streams read their chunks through it and keep their own accept sets and messages: identity also takes strings and shared memory, and compression rejects shared memory, as its spec requires. No behavior change. The helper tests for a view first, the common chunk; the type checks cannot overlap and run no user code, so their order is not observable. A Uint8Array chunk now takes one utils type check in identity instead of three, and two in compression instead of three.
jasnell
force-pushed
the
jasnell/ts-streams-duplicate-consolidaton-4
branch
from
October 14, 2026 18:59
8b44c30 to
80c0ea4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test and consistently handle SharedArrayBuffer and detached ArrayBuffer writes in IdentityTransformStream