worker: handle detached `MessagePort` from a different context #49150

juanarbol · 2023-08-13T19:44:53Z

When worker.moveMessagePortToContext is used, the async handle associated with the port, will be triggered more than needed (at least one more time) and with null data. That can be avoided by simply checking that the data is present and the port is not detached.

Fixes: #49075

Note aside, this could be fixed as well by the next diff:

diff --git a/src/node_messaging.cc b/src/node_messaging.cc
index 00e1db3bab..b46ae022ce 100644
--- a/src/node_messaging.cc
+++ b/src/node_messaging.cc
@@ -741,9 +741,6 @@ MessagePort* MessagePort::New(
     // but it's better to be safe than sorry.)
     Mutex::ScopedLock lock(port->data_->mutex_);
     port->data_->owner_ = port;
-    // If the existing MessagePortData object had pending messages, this is
-    // the easiest way to run that queue.
-    port->TriggerAsync();
   } else if (sibling_group) {
     sibling_group->Entangle(port->data_.get());
   }

IMO that could change how the workers behave, then I prefer a "handle case" approach.

nodejs-github-bot · 2023-08-13T20:37:32Z

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

juanarbol · 2023-08-19T19:48:52Z

Cc @nodejs/workers 😅

src/node_messaging.cc

juanarbol · 2023-08-23T21:00:44Z

@addaleax PTAL

nodejs-github-bot · 2023-08-23T21:02:42Z

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

src/node_messaging.cc

juanarbol

Thanks for reviewing this!

juanarbol · 2023-08-24T15:32:20Z

@addaleax PTAL

nodejs-github-bot · 2023-08-24T15:35:38Z

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

nodejs-github-bot · 2023-08-24T18:43:16Z

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

nodejs-github-bot · 2023-09-04T20:51:41Z

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

nodejs-github-bot · 2023-09-04T22:23:39Z

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

nodejs-github-bot · 2023-09-05T00:16:07Z

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

nodejs-github-bot · 2023-09-05T16:34:36Z

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

nodejs-github-bot · 2023-09-06T04:54:33Z

Commit Queue failed

- Loading data for nodejs/node/pull/49150
✔  Done loading data for nodejs/node/pull/49150
----------------------------------- PR info ------------------------------------
Title      worker: handle detached `MessagePort` from a different context (#49150)
Author     Juan José  (@juanarbol)
Branch     juanarbol:juan/fix-worker-issue -> nodejs:main
Labels     c++, worker, needs-ci
Commits    1
 - worker: handle detached `MessagePort` from a different context
Committers 1
 - Juan José Arboleda 
PR-URL: https://github.com/nodejs/node/pull/49150
Fixes: https://github.com/nodejs/node/issues/49075
Reviewed-By: Anna Henningsen 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/49150
Fixes: https://github.com/nodejs/node/issues/49075
Reviewed-By: Anna Henningsen 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - worker: handle detached `MessagePort` from a different context
   ℹ  This PR was created on Sun, 13 Aug 2023 19:44:53 GMT
   ✔  Approvals: 1
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/49150#pullrequestreview-1593181662
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-09-05T16:34:36Z: https://ci.nodejs.org/job/node-test-pull-request/53747/
- Querying data for job/node-test-pull-request/53747/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/6092861198

juanarbol · 2023-09-06T20:35:59Z

Can anyone from @nodejs/workers review this one? Please, haha

nodejs-github-bot · 2023-10-22T21:20:53Z

Landed in c206b2a

When `worker.moveMessagePortToContext` is used, the async handle associated with the port, will be triggered more than needed (at least one more time) and with null data. That can be avoided by simply checking that the data is present and the port is not detached. Fixes: #49075 Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com> PR-URL: #49150 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

When `worker.moveMessagePortToContext` is used, the async handle associated with the port, will be triggered more than needed (at least one more time) and with null data. That can be avoided by simply checking that the data is present and the port is not detached. Fixes: nodejs#49075 Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com> PR-URL: nodejs#49150 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

When `worker.moveMessagePortToContext` is used, the async handle associated with the port, will be triggered more than needed (at least one more time) and with null data. That can be avoided by simply checking that the data is present and the port is not detached. Fixes: #49075 Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com> PR-URL: #49150 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 13, 2023

juanarbol added worker Issues and PRs related to Worker support. request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Aug 13, 2023

juanarbol force-pushed the juan/fix-worker-issue branch 3 times, most recently from 36978ae to f100bf8 Compare August 13, 2023 20:19

juanarbol added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 13, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 13, 2023

This was referenced Aug 14, 2023

CI Reliability 2023-08-14 nodejs/reliability#636

Open

CI Reliability 2023-08-15 nodejs/reliability#637

Open

CI Reliability 2023-08-16 nodejs/reliability#638

Open

addaleax reviewed Aug 23, 2023

View reviewed changes

src/node_messaging.cc Outdated Show resolved Hide resolved

juanarbol force-pushed the juan/fix-worker-issue branch from f100bf8 to 1d8f4ea Compare August 23, 2023 21:00

juanarbol added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2023

addaleax approved these changes Aug 24, 2023

View reviewed changes

src/node_messaging.cc Outdated Show resolved Hide resolved

juanarbol commented Aug 24, 2023

View reviewed changes

juanarbol force-pushed the juan/fix-worker-issue branch from 1d8f4ea to 4462700 Compare August 24, 2023 15:31

juanarbol added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 24, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 24, 2023

juanarbol added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 24, 2023

github-actions bot mentioned this pull request Sep 4, 2023

CI Reliability 2023-09-04 nodejs/reliability#655

Open

22 tasks

juanarbol added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2023

github-actions bot mentioned this pull request Sep 5, 2023

CI Reliability 2023-09-05 nodejs/reliability#656

Open

19 tasks

github-actions bot mentioned this pull request Sep 6, 2023

CI Reliability 2023-09-06 nodejs/reliability#657

Open

25 tasks

juanarbol added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 6, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 6, 2023

This was referenced Sep 7, 2023

CI Reliability 2023-09-07 nodejs/reliability#658

Open

CI Reliability 2023-09-08 nodejs/reliability#659

Open

CI Reliability 2023-09-09 nodejs/reliability#660

Open

anonrig approved these changes Oct 22, 2023

View reviewed changes

juanarbol added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Oct 22, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 22, 2023

nodejs-github-bot merged commit c206b2a into nodejs:main Oct 22, 2023
67 checks passed

targos mentioned this pull request Oct 23, 2023

v21.1.0 release proposal #50335

Merged

targos mentioned this pull request Nov 12, 2023

v20.10.0 release proposal #50682

Merged

juanarbol deleted the juan/fix-worker-issue branch December 6, 2023 03:27

juanarbol mentioned this pull request Dec 6, 2023

2023-12-06, N|Solid v5.0.1 Iron Release nodesource/nsolid#41

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

worker: handle detached `MessagePort` from a different context #49150

worker: handle detached `MessagePort` from a different context #49150

juanarbol commented Aug 13, 2023

nodejs-github-bot commented Aug 13, 2023

juanarbol commented Aug 19, 2023

juanarbol commented Aug 23, 2023

nodejs-github-bot commented Aug 23, 2023

juanarbol left a comment

juanarbol commented Aug 24, 2023

nodejs-github-bot commented Aug 24, 2023

nodejs-github-bot commented Aug 24, 2023

nodejs-github-bot commented Sep 4, 2023

nodejs-github-bot commented Sep 4, 2023

nodejs-github-bot commented Sep 5, 2023

nodejs-github-bot commented Sep 5, 2023

nodejs-github-bot commented Sep 6, 2023

juanarbol commented Sep 6, 2023 •

edited

Loading

nodejs-github-bot commented Oct 22, 2023

worker: handle detached MessagePort from a different context #49150

worker: handle detached MessagePort from a different context #49150

Conversation

juanarbol commented Aug 13, 2023

nodejs-github-bot commented Aug 13, 2023

juanarbol commented Aug 19, 2023

juanarbol commented Aug 23, 2023

nodejs-github-bot commented Aug 23, 2023

juanarbol left a comment

Choose a reason for hiding this comment

juanarbol commented Aug 24, 2023

nodejs-github-bot commented Aug 24, 2023

nodejs-github-bot commented Aug 24, 2023

nodejs-github-bot commented Sep 4, 2023

nodejs-github-bot commented Sep 4, 2023

nodejs-github-bot commented Sep 5, 2023

nodejs-github-bot commented Sep 5, 2023

nodejs-github-bot commented Sep 6, 2023

juanarbol commented Sep 6, 2023 • edited Loading

nodejs-github-bot commented Oct 22, 2023

worker: handle detached `MessagePort` from a different context #49150

worker: handle detached `MessagePort` from a different context #49150

juanarbol commented Sep 6, 2023 •

edited

Loading