worker: implement `MessagePort` and `MessageChannel`#98

addaleax · 2017-10-08T01:24:26Z

Implement MessagePort and MessageChannel along the lines of
the DOM classes of the same names. MessagePorts initially
support transferring only ArrayBuffers.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added (not yet, but see worker: initial implementation (large/base PR) #58)
commit message follows commit guidelines

Affected core subsystem(s)

worker

hybrist · 2017-10-08T15:58:49Z

src/env.h

 V(decorated_private_symbol, "node:decorated") \
 V(npn_buffer_private_symbol, "node:npnBuffer") \
 V(processed_private_symbol, "node:processed") \
+V(sab_lifetimepartner_symbol, "node:sharedArrayBufferLiftimePartner") \


typo? node:sharedArrayBufferLiftimePartner -> node:sharedArrayBufferLifetimePartner

a) yes, thank you and b) it wasn’t even meant to be in this commit 😄 I’ve fixed it in #58, though

hybrist · 2017-10-09T15:59:08Z

References:

hybrist · 2017-10-09T16:15:57Z

I assume "along the lines" is mostly referring to the differences in event listener interfaces (EventEmitter vs. EventTarget)? Any other intended limitations / deviations?

TimothyGu

More review coming.

TimothyGu · 2017-10-09T17:46:34Z

lib/internal/worker.js

+}
+
+Object.defineProperty(MessagePort.prototype,'onmessage',{
+enumerable: false,


Make it enumerable for consistency.

Hm – the idea here was consistency with ES6 class methods, but I guess you’re right, this should match the native ones

TimothyGu · 2017-10-09T17:47:42Z

src/env.h

 V(promise_wrap_template, v8::ObjectTemplate) \
 V(push_values_to_array_function, v8::Function) \
 V(randombytes_constructor_template, v8::ObjectTemplate) \
+V(sab_lifetimepartner_constructor_template, v8::FunctionTemplate) \


Should this be in this commit?

TimothyGu · 2017-10-09T17:48:10Z

src/node_messaging.cc

+using v8::ValueSerializer;
+
+namespacenode{
+namespaceworker{


Make it namespace messaging for consistency with the file name?

well … it’s still conceptually part of the worker code, isn’t it?

TimothyGu · 2017-10-09T18:10:41Z

@jkrems

I assume "along the lines" is mostly referring to the differences in event listener interfaces (EventEmitter vs. EventTarget)?

There's that, and also some additional APIs like oninit, stop(), ref(), unref(), and hasRef().

TimothyGu · 2017-10-09T18:13:03Z

src/node_messaging.cc

+// Factor generating the MessagePort JS constructor into its own piece
+// of code, because it is needed early on in the child environment setup.
+ Local<FunctionTemplate> templ;
+ templ = env->message_port_constructor_template();


Merge these two lines?

Qard

Other than one question, it all LGTM.

Qard · 2017-10-09T19:16:32Z

lib/internal/worker.js

+this.ref();
+this.start();
+}
+});


Changing onmessage will stop the message and flaggedMessage events from being emitted. Is that intended behaviour?

Yeah, I would say so. This is only there to match the Web spec more closely, but in that case you don’t want the EventEmitter interface anyways I guess.

hybrist · 2017-10-10T03:05:27Z

@TimothyGu For context - I was trying to figure out how hard it would be to write portable code against this, so I wasn't worried too much about additional properties and more about meaningful API overlap. I assume the absence of .addEventListener in node makes this a bit annoying. But short of extending EventEmitter itself I don't think that can be fixed. :(

addaleax · 2017-10-10T03:09:15Z

@jkrems Hm, yes. We could add missing methods to the MessagePort prototype itself, if we want?

hybrist · 2017-10-10T03:22:29Z

I'm somewhat partial towards "make it a clean superset of the existing/spec browser API" but I'm not sure how realistic that is (and how much value it would provide in practice). Adding .addEventListener to these classes might be a neat cheap win in that regard.

addaleax · 2017-10-10T03:24:14Z

@jkrems Do you want to try & take a stab at this yourself? The trickiest thing I guess would be the interaction of all 3 event emitter systems (.on(), .addEventListener(), raw .onmessage = foo)…

Implement `MessagePort` and `MessageChannel` along the lines of the DOM classes of the same names. `MessagePort`s initially support transferring only `ArrayBuffer`s.

Qard · 2017-10-15T19:01:59Z

The missing bits could probably also just be added later.

addaleax · 2017-10-15T19:52:20Z

Landed in f65bb3d

Implement `MessagePort` and `MessageChannel` along the lines of the DOM classes of the same names. `MessagePort`s initially support transferring only `ArrayBuffer`s. PR-URL: #98 Reviewed-By: Stephen Belanger <[email protected]>

Implement `MessagePort` and `MessageChannel` along the lines of the DOM classes of the same names. `MessagePort`s initially support transferring only `ArrayBuffer`s. Thanks to Stephen Belanger for reviewing this change in its original form, to Benjamin Gruenbaum for reviewing the added tests in their original form, and to Olivia Hugger for reviewing the documentation in its original form. Refs: ayojs/ayo#98

Implement `MessagePort` and `MessageChannel` along the lines of the DOM classes of the same names. `MessagePort`s initially support transferring only `ArrayBuffer`s. Thanks to Stephen Belanger for reviewing this change in its original form, to Benjamin Gruenbaum for reviewing the added tests in their original form, and to Olivia Hugger for reviewing the documentation in its original form. Refs: ayojs/ayo#98 PR-URL: #20876 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Shingo Inoue <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

addaleax added the worker label Oct 8, 2017

hybrist reviewed Oct 8, 2017
View reviewed changes

addaleax force-pushed the messageport-start branch from d27a8dc to dd54fc7Compare October 8, 2017 18:19

TimothyGu assigned TimothyGu and unassigned TimothyGu Oct 9, 2017

TimothyGu self-requested a review October 9, 2017 17:42

TimothyGu reviewed Oct 9, 2017
View reviewed changes

TimothyGu self-assigned this Oct 9, 2017

TimothyGu reviewed Oct 9, 2017
View reviewed changes

Qard reviewed Oct 9, 2017
View reviewed changes

worker: implement MessagePort and MessageChannel
d9b49b7
Implement `MessagePort` and `MessageChannel` along the lines of the DOM classes of the same names. `MessagePort`s initially support transferring only `ArrayBuffer`s.

addaleax force-pushed the messageport-start branch from de35803 to c1d07d1Compare October 12, 2017 06:20

addaleax added the semver-minor label Oct 13, 2017

[squash] nits
582b756

addaleax force-pushed the messageport-start branch from c1d07d1 to 582b756Compare October 14, 2017 03:37

Qard approved these changes Oct 15, 2017
View reviewed changes

addaleax closed this Oct 15, 2017

addaleax deleted the messageport-start branch October 15, 2017 19:52

worker: implement MessagePort and MessageChannel#98

worker: implement MessagePort and MessageChannel#98

Uh oh!

Conversation

addaleax commented Oct 8, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hybrist commented Oct 9, 2017

Uh oh!

hybrist commented Oct 9, 2017

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Oct 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Qard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hybrist commented Oct 10, 2017

Uh oh!

addaleax commented Oct 10, 2017

Uh oh!

hybrist commented Oct 10, 2017

Uh oh!

addaleax commented Oct 10, 2017

Uh oh!

Qard commented Oct 15, 2017

Uh oh!

addaleax commented Oct 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

worker: implement `MessagePort` and `MessageChannel`#98

worker: implement `MessagePort` and `MessageChannel`#98