worker: align MessagePort message handling more with Web #26487

addaleax · 2019-03-07T09:32:18Z

This aligns MessagePorts more with the web API and resolved #26463.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2019-03-07T09:32:20Z

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2794/pipeline

addaleax · 2019-03-07T09:38:24Z

/cc @nodejs/workers

CI: https://ci.nodejs.org/job/node-test-pull-request/21299/

test/parallel/test-worker-message-port-onmessage-stop.js

lib/internal/worker/io.js

Trott · 2019-03-07T11:45:15Z

doc/api/worker_threads.md

@@ -203,6 +203,11 @@ input of [`port.postMessage()`][].
 Listeners on this event will receive a clone of the `value` parameter as passed
 to `postMessage()` and no further arguments.

+When there are `'message'` event listeners and `.start()` is not called
+explicitly, and the Web-based `.onmessage` property is not used, no messages


I don't think we capitalize web generally so maybe web-based?

(Maybe web-baed should be removed? It seems likely to be misunderstood. I think we just mean "modeled on the analogous web-based API" but people are likely to interpret as more like...something to do with the web directly? That's what I did when I read this for a few seconds.

(Both comments above are non-blocking.)

I think removing web-based would be a bit confusing, given that we don’t currently document onmessage ourselves.

chjj · 2019-03-07T22:31:10Z

I'd like to look at this more closely.

To keep things clear, the current behavior on master is:

.on('message', fn) with no listeners -> reference and start
.off('message', fn) with one listener -> dereference and stop
.onmessage = fn -> reference and start
.onmessage = null -> dereference and stop

This PR changes the behavior to:

.on('message', fn) with no listeners -> reference and start (unchanged)
.off('message', fn) with one listener -> dereference and stop (unchanged)
.onmessage = fn -> reference and start (unchanged)
.onmessage = null -> dereference, do not stop (changed)

I believe the browser behavior should be:

.on('message', fn) with no listeners -> reference, do not start
.off('message', fn) with one listener -> dereference, do not stop
.onmessage = fn -> reference and start
.onmessage = null -> dereference, do not stop

I'll have to write up some code to confirm that the latter is actually the browser behavior (I haven't actually confirmed that .addEventListener('message') does not start the port, at this point I'm just taking MDN's word for it). It might take me a second.

If that is the behavior, I think there's one more thing to consider: the browser doesn't have references since it's an ever-living event loop (I'm pretty sure workers can't exit gracefully in-browser). That said, we could maybe consider removing the magical referencing behavior of .on('message') and .off('message') altogether since it doesn't actually have anything to do with starting or stopping the port anymore. This would mean that the parentPort would have to be started by default (I believe it is in the browser, since self.start() does not exist, but I'll have to test this as well.

It's probably worth discussing a bit more before anything is merged.

addaleax · 2019-03-07T22:35:56Z

@chjj I think you’re correct about the behaviour everywhere here.

The behaviour for the Node.js-style APIs was picked because it made the most sense to me, and since the browser has neither .on nor .off methods (and the browser lacks EventEmitter as an API and Node.js lacks EventTarget as an API in general), it didn’t seem like a full match was feasible or desirable.

For example, the “event” that the onmessage function receives in Node.js is not really close to being a full MessageEvent.

chjj · 2019-03-07T22:47:46Z

@addaleax, yeah, the more I think about it, the way node does it makes more sense than the browser. In my own worker compatibility module, I actually hack the browser into behaving like node since it seemed to make more sense in the end.

Personally, I'd be fine with having things stay the way they are and simply document the difference (i.e. node.js will automatically start and stop the buffering of the port).

Anyway, I'm going to tinker with the browser more to confirm the exact behavior. Right now, I suspect that addEventListener('message') has no magic behavior and that DedicatedWorkerGlobalScope is akin to a MessagePort started by default.

chjj · 2019-03-07T23:43:13Z

I've confirmed that MDN was correct about port.addEventListener('message'): it does not start the port in the browser.

I've also confirmed that DedicatedWorkerGlobalScope behaves like a MessagePort which is started by default (i.e. you will miss self.onmessage events if you don't bind the listener right away). I don't think it's worth starting the parentPort by default just for browser compat, since there's probably some legitimate use cases for not doing that in node.js.

~~I think the third behavior I described in my first post is probably the way to go if we want browser compat.~~ That said, it might be worth just keeping things the way they are. I do think the current node.js behavior is much saner than the browser. I think my original issue may have been bikeshedding a bit. Would love to hear your thoughts, @addaleax.

edit: I take it all back. I've spent the last hour trying to hash out a sane model which matches the browser. I slowly started diverging from the browser each time something didn't feel quite right until I arrived at node.js's current behavior. Node is right, the browser is wrong.

addaleax · 2019-03-09T11:42:03Z

@chjj So… where would you stand on this PR itself? I agree with you in that the browser behaviour is odd, but I also see value in explicitly aligning with the Web spec here.

Then again, I’m not even sure that the difference is guaranteed to be observable, because in theory receiving a message can take an arbitrary amount of time (and thus just “randomly” happen when a new onmessage listener has been attached).

I’m personally happy to close this and just document the difference, as you suggested above?

chjj · 2019-03-09T18:54:18Z

@addaleax, as far as the PR, I'd say the Cannot send data on closed port error should still be removed, but no changes should be made to the onmessage or .on('message') start/stop behavior. I think the current node behavior is good. Documenting the differences from the browser is probably sufficient.

This aligns `MessagePort`s more with the web API. Refs: nodejs#26463

addaleax · 2019-03-10T11:14:04Z

@chjj Okay, I think we’re in agreement – I’ve removed the changes for .onmessage and added docs for the divergence from the browser.

@gireeshpunathil @benjamingr Do you want to take another look?

gireeshpunathil · 2019-03-10T12:36:28Z

still LGTM.

BridgeAR · 2019-03-10T20:55:03Z

CI https://ci.nodejs.org/job/node-test-pull-request/21404/ (:heavy_check_mark:)

addaleax · 2019-03-11T09:59:05Z

Landed in a52aede, 137d3d2

This aligns `MessagePort`s more with the web API. Refs: #26463 PR-URL: #26487 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Fixes: #26463 PR-URL: #26487 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

This aligns `MessagePort`s more with the web API. Refs: #26463 PR-URL: #26487 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Fixes: #26463 PR-URL: #26487 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

This aligns `MessagePort`s more with the web API. Refs: #26463 PR-URL: #26487 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Fixes: #26463 PR-URL: #26487 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. worker Issues and PRs related to Worker support. labels Mar 7, 2019

addaleax force-pushed the worker-messageport-noerror branch from 365a595 to 8354833 Compare March 7, 2019 09:34

gireeshpunathil reviewed Mar 7, 2019

View reviewed changes

test/parallel/test-worker-message-port-onmessage-stop.js Outdated Show resolved Hide resolved

test/parallel/test-worker-message-port-onmessage-stop.js Outdated Show resolved Hide resolved

lib/internal/worker/io.js Outdated Show resolved Hide resolved

Trott reviewed Mar 7, 2019

View reviewed changes

benjamingr approved these changes Mar 7, 2019

View reviewed changes

gireeshpunathil approved these changes Mar 7, 2019

View reviewed changes

addaleax added 2 commits March 10, 2019 12:12

worker: remove ERR_CLOSED_MESSAGE_PORT

060b524

This aligns `MessagePort`s more with the web API. Refs: nodejs#26463

doc: document diverging MessagePort.onmessage handling

77f6b8c

addaleax force-pushed the worker-messageport-noerror branch from ab4fae7 to 77f6b8c Compare March 10, 2019 11:13

jasnell approved these changes Mar 10, 2019

View reviewed changes

BridgeAR approved these changes Mar 10, 2019

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 10, 2019

addaleax closed this Mar 11, 2019

addaleax deleted the worker-messageport-noerror branch March 11, 2019 09:59

BridgeAR mentioned this pull request Mar 13, 2019

2019-03-14, Version 11.12.0 (Current) #26644

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

worker: align MessagePort message handling more with Web #26487

worker: align MessagePort message handling more with Web #26487

addaleax commented Mar 7, 2019

nodejs-github-bot commented Mar 7, 2019

addaleax commented Mar 7, 2019

Trott Mar 7, 2019 •

edited

Loading

Trott Mar 7, 2019

Trott Mar 7, 2019

addaleax Mar 7, 2019

chjj commented Mar 7, 2019 •

edited

Loading

addaleax commented Mar 7, 2019

chjj commented Mar 7, 2019

chjj commented Mar 7, 2019 •

edited

Loading

addaleax commented Mar 9, 2019

chjj commented Mar 9, 2019

addaleax commented Mar 10, 2019

gireeshpunathil commented Mar 10, 2019

BridgeAR commented Mar 10, 2019 •

edited by addaleax

Loading

addaleax commented Mar 11, 2019

worker: align MessagePort message handling more with Web #26487

worker: align MessagePort message handling more with Web #26487

Conversation

addaleax commented Mar 7, 2019

Checklist

nodejs-github-bot commented Mar 7, 2019

addaleax commented Mar 7, 2019

Trott Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

Trott Mar 7, 2019

Choose a reason for hiding this comment

Trott Mar 7, 2019

Choose a reason for hiding this comment

addaleax Mar 7, 2019

Choose a reason for hiding this comment

chjj commented Mar 7, 2019 • edited Loading

addaleax commented Mar 7, 2019

chjj commented Mar 7, 2019

chjj commented Mar 7, 2019 • edited Loading

addaleax commented Mar 9, 2019

chjj commented Mar 9, 2019

addaleax commented Mar 10, 2019

gireeshpunathil commented Mar 10, 2019

BridgeAR commented Mar 10, 2019 • edited by addaleax Loading

addaleax commented Mar 11, 2019

Trott Mar 7, 2019 •

edited

Loading

chjj commented Mar 7, 2019 •

edited

Loading

chjj commented Mar 7, 2019 •

edited

Loading

BridgeAR commented Mar 10, 2019 •

edited by addaleax

Loading