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: handle detached MessagePort from a different context #49150

Merged
merged 1 commit into from
Oct 22, 2023

Conversation

juanarbol
Copy link
Member

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 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 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 juanarbol force-pushed the juan/fix-worker-issue branch 3 times, most recently from 36978ae to f100bf8 Compare August 13, 2023 20:19
@juanarbol juanarbol added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 13, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 13, 2023
@nodejs-github-bot
Copy link
Collaborator

@juanarbol
Copy link
Member Author

Cc @nodejs/workers 😅

src/node_messaging.cc Outdated Show resolved Hide resolved
@juanarbol juanarbol force-pushed the juan/fix-worker-issue branch from f100bf8 to 1d8f4ea Compare August 23, 2023 21:00
@juanarbol juanarbol added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2023
@juanarbol
Copy link
Member Author

@addaleax PTAL

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

src/node_messaging.cc Outdated Show resolved Hide resolved
Copy link
Member Author

@juanarbol juanarbol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing this!

@juanarbol juanarbol force-pushed the juan/fix-worker-issue branch from 1d8f4ea to 4462700 Compare August 24, 2023 15:31
@juanarbol juanarbol added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 24, 2023
@juanarbol
Copy link
Member Author

@addaleax PTAL

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

@nodejs-github-bot
Copy link
Collaborator

@juanarbol juanarbol added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 24, 2023
@juanarbol juanarbol added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@juanarbol juanarbol added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 6, 2023
@nodejs-github-bot 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
@nodejs-github-bot
Copy link
Collaborator

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
Copy link
Member Author

juanarbol commented Sep 6, 2023

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

@juanarbol 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 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 nodejs-github-bot merged commit c206b2a into nodejs:main Oct 22, 2023
3 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in c206b2a

targos pushed a commit that referenced this pull request Oct 23, 2023
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>
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
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>
targos pushed a commit that referenced this pull request Nov 11, 2023
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>
@juanarbol juanarbol deleted the juan/fix-worker-issue branch December 6, 2023 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segmentation Fault on Contextifying MessagePort Transferred to a Worker Thread
4 participants