Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

worker_threads: inconsistent stop/start behavior with MessagePort.onmessage #26463

Closed
chjj opened this issue Mar 6, 2019 · 0 comments
Closed
Assignees
Labels
worker Issues and PRs related to Worker support.

Comments

@chjj
Copy link
Contributor

chjj commented Mar 6, 2019

  • Version: v11.10.1
  • Platform: Linux 4.20.11-arch1-1-ARCH deps: update openssl to 1.0.1j #1 SMP PREEMPT Wed Feb 20 21:11:12 UTC 2019 x86_64 GNU/Linux
  • Subsystem: worker_threads

These two samples of code differ in output between node.js and the browser:

Browser (Chromium 72.0.3626.119):

const {MessageChannel} = global;
const {port1, port2} = new MessageChannel();

port1.postMessage('hello');

setTimeout(() => {
  port2.onmessage = ({data}) => {
    console.log(data);

    port2.onmessage = null;
    port1.postMessage('world');

    setTimeout(() => {
      port2.onmessage = ({data}) => {
        console.log(data);
      };
    }, 100);
  };
}, 100);

Output:

hello

node.js with identical logic:

const {MessageChannel} = require('worker_threads');
const {port1, port2} = new MessageChannel();

port1.postMessage('hello');

setTimeout(() => {
  port2.onmessage = (data) => {
    console.log(data);

    port2.onmessage = null;
    port1.postMessage('world');

    setTimeout(() => {
      port2.onmessage = (data) => {
        console.log(data);
      };
    }, 100);
  };
}, 100);

Output:

hello
world

The reason for this is that node.js calls stopMessagePort() [1] [2] [3] when the onmessage listener is removed (likewise for .on('message')). This causes the port to start buffering messages again and is the reason why we can still get world once we rebind the event listener. The browser does not implement this behavior: once a MessagePort is started, it is started forever until closed.

(Another small thing to consider: port.onmessage = () => {} starts the port in the browser whereas port.addEventListener('message', () => {}) does not.)

The second strange thing that I've found, which is sort of related to the stopMessagePort() behavior involves closing MessagePorts.

Here is an example:

const {MessageChannel} = require('worker_threads');
const {port1, port2} = new MessageChannel();

port1.on('message', console.log);
port1.close(() => {
  port1.off('message', console.log);
});

Throws:

$ node port.js
internal/worker/io.js:155
      MessagePortPrototype.stop.call(port);
                                ^

Error: Cannot send data on closed MessagePort
    at MessagePort.eventEmitter.on (internal/worker/io.js:155:33)
    at MessagePort.emit (events.js:197:13)
    at MessagePort.removeListener (events.js:333:18)
    at MessagePort.port1.close (/home/chjj/port.js:6:9)
    at Object.onceWrapper (events.js:285:13)
    at MessagePort.emit (events.js:197:13)
    at MessagePort.onclose (internal/worker/io.js:105:8)

There seems to be a small window of time where removing the message listener will throw after the port is closed (putting the port1.off(...) call in a setTimeout doesn't seem to produce the error) [4] [5]. The browser never throws when removing events (partially because it doesn't implement this auto-stop behavior).

chjj added a commit to chjj/bthreads that referenced this issue Mar 6, 2019
chjj added a commit to chjj/bthreads that referenced this issue Mar 6, 2019
@addaleax addaleax added the worker Issues and PRs related to Worker support. label Mar 6, 2019
addaleax added a commit to addaleax/node that referenced this issue Mar 10, 2019
This aligns `MessagePort`s more with the web API.

Refs: nodejs#26463
addaleax added a commit that referenced this issue Mar 11, 2019
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]>
BridgeAR pushed a commit that referenced this issue Mar 13, 2019
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]>
BridgeAR pushed a commit that referenced this issue Mar 13, 2019
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]>
BridgeAR pushed a commit that referenced this issue Mar 14, 2019
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]>
BridgeAR pushed a commit that referenced this issue Mar 14, 2019
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]>
juanarbol added a commit to juanarbol/node that referenced this issue May 3, 2021
This reverts commit 73370b4.

The unit test is preserved to make sure it does not break
nodejs#26463 again.
jasnell pushed a commit that referenced this issue May 6, 2021
This reverts commit 73370b4.

The unit test is preserved to make sure it does not break
#26463 again.

PR-URL: #38510
Fixes: #38499
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue May 17, 2021
This reverts commit 73370b4.

The unit test is preserved to make sure it does not break
#26463 again.

PR-URL: #38510
Fixes: #38499
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue May 30, 2021
This reverts commit 73370b4.

The unit test is preserved to make sure it does not break
#26463 again.

PR-URL: #38510
Fixes: #38499
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Jun 5, 2021
This reverts commit 73370b4.

The unit test is preserved to make sure it does not break
#26463 again.

PR-URL: #38510
Fixes: #38499
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Jun 5, 2021
This reverts commit 73370b4.

The unit test is preserved to make sure it does not break
#26463 again.

PR-URL: #38510
Fixes: #38499
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Jun 11, 2021
This reverts commit 73370b4.

The unit test is preserved to make sure it does not break
#26463 again.

PR-URL: #38510
Fixes: #38499
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants