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

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

src/env.h

@@ -92,6 +92,7 @@ class ModuleWrap;
  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

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

References:

jkrems · 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

@@ -317,6 +322,7 @@ class ModuleWrap;
  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;
+
+namespace node {
+namespace worker {


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.

jkrems · 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?

jkrems · 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

jkrems reviewed Oct 8, 2017

View reviewed changes

addaleax force-pushed the messageport-start branch from d27a8dc to dd54fc7 Compare 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 c1d07d1 Compare 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 582b756 Compare 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

addaleax commented Oct 8, 2017

jkrems Oct 8, 2017

addaleax Oct 8, 2017

jkrems commented Oct 9, 2017

jkrems commented Oct 9, 2017

TimothyGu left a comment

TimothyGu Oct 9, 2017

addaleax Oct 10, 2017

TimothyGu Oct 9, 2017

addaleax Oct 10, 2017

TimothyGu Oct 9, 2017

addaleax Oct 10, 2017

TimothyGu commented Oct 9, 2017

TimothyGu Oct 9, 2017

addaleax Oct 10, 2017

Qard left a comment

Qard Oct 9, 2017

addaleax Oct 10, 2017

jkrems commented Oct 10, 2017

addaleax commented Oct 10, 2017

jkrems commented Oct 10, 2017

addaleax commented Oct 10, 2017

Qard commented Oct 15, 2017

addaleax commented Oct 15, 2017

worker: implement MessagePort and MessageChannel #98

worker: implement MessagePort and MessageChannel #98

Conversation

addaleax commented Oct 8, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkrems commented Oct 9, 2017

jkrems commented Oct 9, 2017

TimothyGu 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

Choose a reason for hiding this comment

TimothyGu commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Qard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkrems commented Oct 10, 2017

addaleax commented Oct 10, 2017

jkrems commented Oct 10, 2017

addaleax commented Oct 10, 2017

Qard commented Oct 15, 2017

addaleax commented Oct 15, 2017

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

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