streams: add check transfer option #59610
Conversation
|
Review requested:
|
nodejs-github-bot
added
needs-ci
hotpineapple
force-pushed
the
implement-transform-stream-memeber-check
branch
from
5d4342b to
4b20401
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #59610 +/- ##
==========================================
- Coverage 89.89% 89.85% -0.05%
==========================================
Files 667 667
Lines 195320 196191 +871
Branches 38349 38549 +200
==========================================
+ Hits 175578 176281 +703
- Misses 12217 12361 +144
- Partials 7525 7549 +24
🚀 New features to boost your workflow:
|
| @@ -123,6 +125,18 @@ function structuredClone(value, options) { | |||
| }, | |||
| ); | |||
|
|
|||
| const transferList = idlOptions.transfer; | |||
| for (let i = 0; i < transferList.length; i++) { | |||
There was a problem hiding this comment.
we should use ArrayPrototypeForEach to make it resilient to prototype pollution
hotpineapple
force-pushed
the
implement-transform-stream-memeber-check
branch
from
4b20401 to
6559712
Compare
check if there is transform stream and its member in transfer list
| if (isTransformStream(outer)) { | ||
| ArrayPrototypeForEach(transferList, (inner, j) => { | ||
| if (i !== j && (inner === outer.readable || inner === outer.writable)) { | ||
| throw new DOMException('Cannot transfer a stream and its members', 'DataCloneError'); |
There was a problem hiding this comment.
These steps aren't explicitly listed in the specification for structuredClone. It also seems weird to handle a highly specific edge case like this here, especially since there are many other ways than just structuredClone that can trigger a transfer, e.g. postMessage. If we really needed a workaround specific to TransformStreams, I would expect to find it near the implementation of its transfer steps, so that it is handled correctly regardless of how the transfer was triggered.
But to be honest, I don't think we should be fixing the test failure by adding extra code to detect this exact edge case. The streams specification requires that the transfer fails if the readable or writable side of the TransformStream is locked, which should be the case if either side was transferred before. It seems like Node's current implementation doesn't do these checks correctly, and we should investigate and fix the root cause of that problem instead.
There was a problem hiding this comment.
Looking a bit closer, I believe the transfer steps for TransformStream are not implemented correctly. Steps 3 and 4 of the specification require readable and writable to be structurally cloned during the transfer steps of TransformStream. However, Node's implementation simply creates a data object with the not-yet-transferred readable and writable streams... 🤔
There was a problem hiding this comment.
@MattiasBuelens I really appreciate your detailed explanation! I'll review it again based on what you said.🙂
check if there is transform stream and its member in transfer list
this make wpt test transform-stream-members.any.js pass