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

MessagePort can send messages after closed #42296

Closed
jrvidal opened this issue Mar 11, 2022 · 5 comments · Fixed by #42357
Closed

MessagePort can send messages after closed #42296

jrvidal opened this issue Mar 11, 2022 · 5 comments · Fixed by #42357
Labels
worker Issues and PRs related to Worker support.

Comments

@jrvidal
Copy link
Contributor

jrvidal commented Mar 11, 2022

Version

v17.7.1

Platform

Linux 5.13.0-30-generic #33~20.04.1-Ubuntu SMP Mon Feb 7 14:25:10 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

worker_threads

What steps will reproduce the bug?

const { MessageChannel } = require('worker_threads');

const ch = new MessageChannel()

ch.port1.onmessage = () => console.log('hey');

ch.port2.close();

ch.port2.postMessage('asdf');

How often does it reproduce? Is there a required condition?

Consistently in v17.7.1, v16.14.0 and v14.19.0.

What is the expected behavior?

The program should not log anything, since port2 is closed right before the call to postMessage.

What do you see instead?

The program does log hey.

Additional information

For whatever is worth, both in Chrome (Chromium 98.0.4758.102, V8 9.8.177.11) and Firefox (97.0.2), the message never arrives.

@VoltrexKeyva VoltrexKeyva added the worker Issues and PRs related to Worker support. label Mar 11, 2022
@WillianAgostini
Copy link

Hi @jrvidal

In this case the method ch.port2.close() has not been completely closed when the code calls ch.port2.postMessage('asdf').

If you execute the next code you can see the log 'hey' has shown before than log 'close'.

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

port1.onmessage = () => console.log('hey');
port2.on('close', () => console.log('fechou'));

port2.close();
port2.postMessage('asdf');

You can see this docs on https://nodejs.org/api/worker_threads.html#portclose.

@jrvidal
Copy link
Contributor Author

jrvidal commented Mar 14, 2022

You can see this docs on https://nodejs.org/api/worker_threads.html#portclose.

Thanks, the docs are somewhat ambiguous about this point, that's why I reported.

I think it'd be desirable to more closely align with browsers here, or at least be more explicit about this behavior in the documentation.

@Trott
Copy link
Member

Trott commented Mar 16, 2022

#42357 aligns the behavior with the expectations described here and the behavior described for Chrome and Firefox.

Trott added a commit to Trott/io.js that referenced this issue Mar 16, 2022
@jrvidal
Copy link
Contributor Author

jrvidal commented Mar 16, 2022

@Trott Thanks for that PR! Any intent to backport to 16.x?

@aduh95
Copy link
Contributor

aduh95 commented Mar 16, 2022

Any intent to backport to 16.x?

Node.js v16.x is on Active LTS mode; unless a git conflict arises, it should be backported "automatically" after the adequate maturation delay, see

what can be landed. Our LTS release lines (see the [Release Plan][])
require that commits mature in the Current release for at least 2 weeks before
they can be landed in an LTS staging branch. Only after "maturation" will those
commits be cherry-picked or backported.

@Trott Trott closed this as completed in 44131ad Mar 18, 2022
bengl pushed a commit that referenced this issue Mar 21, 2022
Fixes: #42296

PR-URL: #42357
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit to danielleadams/node that referenced this issue Apr 21, 2022
Fixes: nodejs#42296

PR-URL: nodejs#42357
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit that referenced this issue Apr 24, 2022
Fixes: #42296

PR-URL: #42357
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit that referenced this issue Apr 24, 2022
Fixes: #42296

PR-URL: #42357
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit that referenced this issue Apr 24, 2022
Fixes: #42296

PR-URL: #42357
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
xtx1130 pushed a commit to xtx1130/node that referenced this issue Apr 25, 2022
Fixes: nodejs#42296

PR-URL: nodejs#42357
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
juanarbol pushed a commit that referenced this issue Apr 28, 2022
Fixes: #42296

PR-URL: #42357
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
juanarbol pushed a commit that referenced this issue May 1, 2022
Fixes: #42296

PR-URL: #42357
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
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.

6 participants