Support transferable streams (postMessage) #1053

ricea · 2020-07-08T19:27:19Z

Make readable, writable, and transform streams transferable via
postMessage(stream, [stream]).

The streams themselves must be transferred, but the chunks written or
read from the streams are cloned, not transferred. Support for
transferring chunks will require API changes and is expected to be added
in a future update.

There is no reference implementation of this functionality as jsdom does
not support transferable objects, and so it wouldn't be testable.

Closes #276.

At least two implementers are interested (and none opposed):
- Chrome
- In a private email, @youennf expressed support for shipping the current approach
Tests are written and can be reviewed and commented upon at:
- Add tests for transferable streams web-platform-tests/wpt#24546
Implementation bugs are filed:
- Chrome: https://crbug.com/894838
- Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1659025
- Safari: https://bugs.webkit.org/show_bug.cgi?id=215485

(See WHATWG Working Mode: Changes for more details.)

Preview | Diff

ricea · 2020-07-08T19:30:29Z

This is ready for review. @domenic and @MattiasBuelens PTAL.

There's a minor issue that I haven't figured out how to link {{MessagePort}}'s transfer steps. Probably it will require an editorial change to the HTML standard, or maybe it's not worth the effort.

I need to check I don't have any gaps in my tests and port them to wpt, but I don't expect that to take long.

ricea · 2020-07-08T19:53:17Z

Tip for easier reviewing: The changes are under
https://whatpr.org/streams/1053.html#rs-transfer
https://whatpr.org/streams/1053.html#ws-transfer
https://whatpr.org/streams/1053.html#ts-transfer
https://whatpr.org/streams/1053.html#transferrable-streams

MattiasBuelens

Looks pretty good! 🙂

From your comment on #276, I understand that we will not yet support the "double-transfer" case in this first version. Correct?

There is no reference implementation of this functionality as jsdom does not support transferrable objects, and so it wouldn't be testable.

That is indeed unfortunate.

For remote-web-streams, I wrote a mock implementation of MessageChannel. It doesn't actually do any (de)serialization or send stuff between realms, it's just enough so I can run my tests. But I guess that wouldn't really work for a project like jsdom... 😅

index.bs

MattiasBuelens · 2020-07-08T20:07:37Z

index.bs

+   1. Perform ! [$WritableStreamDefaultControllerErrorIfNeeded$](|controller|, |value|).
+   1. If |backpressurePromise| is not undefined,
+    1. [=Resolve=] |backpressurePromise| with undefined.
+    1. Set |backpressurePromise| to undefined.


It's my understanding that we only need to disentangle the port on one side. Although from the implementers' perspective it might be better to explicitly disentangle on both sides. Let me think about it.

I'm leaning towards "no" now, since in platonic spec-land it is redundant to disentangle from both sides, and in practice implementations need to implement MessagePort close() in such a way that both sides get closed after all queued messages have been delivered.

index.bs

ricea · 2020-07-08T20:58:54Z

From your comment on #276, I understand that we will not yet support the "double-transfer" case in this first version. Correct?

Correct. It's unfortunate, but I said there, it's very complex to support.

That is indeed unfortunate.

For remote-web-streams, I wrote a mock implementation of MessageChannel. It doesn't actually do any (de)serialization or send stuff between realms, it's just enough so I can run my tests. But I guess that wouldn't really work for a project like jsdom... 😅

We could polyfill MessageChannel and MessagePort I suppose, but we'd need quite a high level of fidelity and at that point it might be easier just to implement it in jsdom. Anyway, I'd prefer not to block on that.

domenic

Mostly-editorial feedback. It's great to see this coming together!

index.bs

domenic · 2020-07-09T19:13:16Z

index.bs

+ 1. Perform ! [$InitializeReadableStream$](|stream|).
+ 1. Let |controller| be a [=new=] {{ReadableStreamDefaultController}}.
+ 1. Let |backpressurePromise| be [=a new promise=].
+ 1. Add a handler for |port|'s {{MessagePort/message}} event with the following steps:


"Add a handler" is not well-defined. I've run into this problem before; you need something like https://wicg.github.io/kv-storage/#add-a-simple-event-listener. Maybe we can upstream that into DOM.

It feels odd having to route the event via JavaScript in the standard when in Blink's implementation we just have blink::NativeEventListener. And it's not like it's some weird edge-case either: it's used in loads of places.

Yeah. To be clear I don't suggest you change anything here, but I'd like to get us better primitives.

index.bs

domenic · 2020-07-09T19:15:10Z

index.bs

+    1. Set |backpressurePromise| to [=a new promise=].
+  1. Otherwise, if |type| is `"close"`,
+    1. Perform ! [$ReadableStreamDefaultControllerClose$](|controller|).
+    1. Disentangle |port|.


Wow, it's weird that this is not a defined term.

Yes, I was confused when I didn't find a definition.

index.bs

The Streams Standard change whatwg/streams#1053 adds the ability to transfer streams to a different realm using postMessage(). Add tests for this feature.

ricea · 2020-07-09T23:51:03Z

I've uploaded the tests at web-platform-tests/wpt#24546. I've left it as a draft PR until we are ready to land this, but please feel free to review anyway.

index.bs

domenic · 2020-07-13T18:51:20Z

index.bs

+ <dfn abstract-op lt="PackAndPostMessage">PackAndPostMessage(|port|, |type|, |value|)</dfn> performs
+ the following steps:
+
+ 1. Let |message| be [$OrdinaryObjectCreate$](null).


Suggested change

1. Let |message| be [$OrdinaryObjectCreate$](null).

1. Let |message| be ! [$OrdinaryObjectCreate$](null).

OrdinaryObjectCreate does not return a completion record. This is non-obvious from the definition https://tc39.es/ecma262/2020/#sec-ordinaryobjectcreate but clear from how it is used elsewhere: https://tc39.es/ecma262/2020/#sec-createiterresultobject.

Everything returns a completion record: https://tc39.es/ecma262/#sec-implicit-completion-values

domenic · 2020-07-13T18:52:11Z

index.bs

+ 1. Perform ! [$CreateDataProperty$](|message|, "`type`", |type|).
+ 1. Perform ! [$CreateDataProperty$](|message|, "`value`", |value|).
+ 1. Let |targetPort| be the port with which |port| is entangled, if any; otherwise let it be null.
+ 1. Let |options| be «[ "`transfer`" → « » ]».


TODO for me to make these linkable

index.bs

domenic

LGTM; very nice.

We should work on getting a second implementer signoff, especially on the remaining open questions in #276.

domenic · 2020-07-14T16:51:12Z

index.bs

+ 1. If ! [$IsReadableStreamLocked$](|readable|) is true, throw a "{{DataCloneError}}"
+    {{DOMException}}.
+ 1. If ! [$IsWritableStreamLocked$](|writable|) is true, throw a "{{DataCloneError}}"
+    {{DOMException}}.


Adding these checks here, as opposed to delegating to transferring the readable and writable sides independently, avoids ending up with a "half-transferred" transform stream. That's nice :). I wonder if we should <p class="note"> that? In either case we should be sure we have a test for it.

I've updated the tests to test this feature (which is missing from Blink's implementation 😳).

I don't feel I need to call attention to the feature with a note, as it is just a matter of polish.

index.bs

ricea · 2020-07-21T20:42:31Z

Can I say that Google Chrome is interested even though I'm the one who wrote the change? 😄

@jorendorff and @youennf could I hear your views on this?

domenic · 2020-07-21T20:45:25Z

Yes, you certainly can :)

ricea · 2020-07-21T21:25:15Z

Google Chrome is interested!

ricea · 2020-08-13T22:07:42Z

@MattiasBuelens Github thinks I have unresolved comments from you, but I can't tell which they are. Are you okay with landing this, or do you think it needs more work?

MattiasBuelens

LGTM. 👍

Perhaps it's because this review comment has a "suggested change" in it that wasn't committed? Anyway, you can ignore that.

Make readable, writable, and transform streams transferable via postMessage(stream, [stream]). The streams themselves must be transferred, but the chunks written or read from the streams are cloned, not transferred. Support for transferring chunks will require API changes and is expected to be added in a future update. There is no reference implementation of this functionality as jsdom does not support transferrable objects, and so it wouldn't be testable. Closes whatwg#276.

Co-authored-by: Mattias Buelens <[email protected]>

Co-authored-by: Domenic Denicola <[email protected]>

Co-authored-by: Mattias Buelens <[email protected]>

The Streams Standard change whatwg/streams#1053 adds the ability to transfer streams to a different realm using postMessage(). Add tests for this feature.

…stonly Automatic update from web-platform-tests Add tests for transferable streams (#24546) The Streams Standard change whatwg/streams#1053 adds the ability to transfer streams to a different realm using postMessage(). Add tests for this feature. -- wpt-commits: f2eb8b9fa0d0d11429ce4952dbb9dd796c18e0da wpt-pr: 24546

MattiasBuelens requested changes Jul 8, 2020

View reviewed changes

domenic reviewed Jul 9, 2020

View reviewed changes

ricea added a commit to ricea/web-platform-tests that referenced this pull request Jul 9, 2020

Add tests for transferable streams

e806f4d

The Streams Standard change whatwg/streams#1053 adds the ability to transfer streams to a different realm using postMessage(). Add tests for this feature.

ricea mentioned this pull request Jul 9, 2020

Add tests for transferable streams web-platform-tests/wpt#24546

Merged

domenic reviewed Jul 13, 2020

View reviewed changes

domenic mentioned this pull request Jul 13, 2020

Allow other specs to add event listeners ergonomically whatwg/dom#878

Open

domenic approved these changes Jul 14, 2020

View reviewed changes

domenic mentioned this pull request Jul 14, 2020

can streams be transferred via postMessage()? #276

Closed

MattiasBuelens reviewed Jul 18, 2020

View reviewed changes

index.bs Outdated Show resolved Hide resolved

ricea mentioned this pull request Jul 21, 2020

Communication between workers/windows via Streams API #244

Open

ricea force-pushed the transferable-streams branch from 3762b86 to 8dd6ff6 Compare August 13, 2020 18:28

MattiasBuelens approved these changes Aug 13, 2020

View reviewed changes

ricea and others added 10 commits August 14, 2020 08:40

Use American spelling for "uninitialized"

fdb3444

Add missing |error| argument.

ecad896

Co-authored-by: Mattias Buelens <[email protected]>

Add missing "."

0444bdc

Co-authored-by: Domenic Denicola <[email protected]>

Move the transfer steps into separate sections

00fa25c

Update text for creating MessagePorts

86eed13

Move IsLocked checks earlier and throw a DataCloneError

780a016

Remove pointless Type settings for TransformStream

d239fd5

Initialise all slots in transferred TransformStream

6dfb817

Add missing "!"

353484a

ricea and others added 20 commits August 14, 2020 08:43

Clarify that it is Object.prototype we are worred about

225dcc4

Change paranoid checks to asserts, and add security warning

69c1a58

Remove optional "undefined" from call to PipeTo()

0168dfd

Use StructuredSerializeWithTransfer() to transfer MessagePort

5af0984

Explicitly enable port message queue

bbd6686

Add missing "."s

b09c403

Fix string quoting ("s" -> "s")

f6c032f

Remove backpressurePromise from CrossRealmTransformReadable

7ee6687

Link {{ReadableStream}}

7203f36

Co-authored-by: Domenic Denicola <[email protected]>

Put postMessage() in code font

5c94076

Co-authored-by: Domenic Denicola <[email protected]>

Add missing formatting of port2 as var.

86d347a

Co-authored-by: Domenic Denicola <[email protected]>

Add Oxford comma

dbb2fb4

Co-authored-by: Domenic Denicola <[email protected]>

Improve postMessage examples a bit

1370e5f

Apply editorial tweaks to WritableStream and TransformStream sections

07df257

Use high-level operations in TransformStream transfer steps

cf47bfc

Alphabetize the abstract ops

6a8b363

Make PackAndPostMessageHandlingError return a completion record

6e53603

Nits

1a76913

Fix bad link text for "locked"

017d200

Co-authored-by: Mattias Buelens <[email protected]>

Linkify slots

a5d56a9

ricea force-pushed the transferable-streams branch from 8dd6ff6 to a5d56a9 Compare August 14, 2020 01:36

ricea merged commit 1fd19a0 into whatwg:master Aug 14, 2020

ricea deleted the transferable-streams branch August 14, 2020 02:11

ricea mentioned this pull request Aug 14, 2020

Transferable Streams mozilla/standards-positions#430

Closed

annevk mentioned this pull request Oct 26, 2020

Serialise with implicit transfer (for Streams) whatwg/html#3684

Closed

MattiasBuelens mentioned this pull request Dec 1, 2021

Editorial: align .webidl files with WebIDL from spec text #1192

Merged

domenic mentioned this pull request Apr 13, 2023

Add support for ReadableStream "owning" type #1271

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support transferable streams (postMessage) #1053

Support transferable streams (postMessage) #1053

ricea commented Jul 8, 2020 •

edited by pr-preview bot

Loading

ricea commented Jul 8, 2020

ricea commented Jul 8, 2020 •

edited

Loading

MattiasBuelens left a comment

MattiasBuelens Jul 8, 2020

ricea Jul 8, 2020

ricea Jul 10, 2020

ricea commented Jul 8, 2020

domenic left a comment

domenic Jul 9, 2020

ricea Jul 9, 2020

domenic Jul 9, 2020

domenic Jul 9, 2020

ricea Jul 9, 2020

ricea commented Jul 9, 2020

domenic Jul 13, 2020

ricea Jul 14, 2020

domenic Jul 14, 2020

domenic Jul 13, 2020

domenic left a comment

domenic Jul 14, 2020

ricea Jul 15, 2020

ricea commented Jul 21, 2020

domenic commented Jul 21, 2020

ricea commented Jul 21, 2020

ricea commented Aug 13, 2020

MattiasBuelens left a comment

	1. Set \|backpressurePromise\| to undefined.
	1. Set \|backpressurePromise\| to undefined.
	1. Disentangle \|port\|.

	1. Let \|message\| be [$OrdinaryObjectCreate$](null).
	1. Let \|message\| be ! [$OrdinaryObjectCreate$](null).

Support transferable streams (postMessage) #1053

Support transferable streams (postMessage) #1053

Conversation

ricea commented Jul 8, 2020 • edited by pr-preview bot Loading

ricea commented Jul 8, 2020

ricea commented Jul 8, 2020 • edited Loading

MattiasBuelens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricea commented Jul 8, 2020

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricea commented Jul 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricea commented Jul 21, 2020

domenic commented Jul 21, 2020

ricea commented Jul 21, 2020

ricea commented Aug 13, 2020

MattiasBuelens left a comment

Choose a reason for hiding this comment

ricea commented Jul 8, 2020 •

edited by pr-preview bot

Loading

ricea commented Jul 8, 2020 •

edited

Loading