@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

/cc @nodejs/workers

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

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.

@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.

@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.

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.

@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?

@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.

@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?

still LGTM.

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

Landed in a52aede, 137d3d2