GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — worker: handle detached `MessagePort` from a different context by juanarbol · Pull Request #49150 · nodejs/node

Conversation

@juanarbol

@juanarbol juanarbol commented Aug 13, 2023

Copy link
Copy Markdown
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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@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 the worker_threads module and Worker API. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. and removed request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. 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. Only starts once the PR has an approving review. label Aug 13, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 13, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 13, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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
@juanarbol

juanarbol commented Aug 19, 2023

Copy link
Copy Markdown
Member Author

Cc @nodejs/workers 😅
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax reviewed Aug 23, 2023
View reviewed changes
Comment thread src/node_messaging.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@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. Only starts once the PR has an approving review. label Aug 23, 2023
@juanarbol

juanarbol commented Aug 23, 2023

Copy link
Copy Markdown
Member Author

@addaleax PTAL
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 23, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 23, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax approved these changes Aug 24, 2023
View reviewed changes
Comment thread src/node_messaging.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
juanarbol
juanarbol commented Aug 24, 2023
View reviewed changes

@juanarbol juanarbol left a comment

Copy link
Copy Markdown
Member Author

There was a problem hiding this 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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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. Only starts once the PR has an approving review. label Aug 24, 2023
@juanarbol

juanarbol commented Aug 24, 2023

Copy link
Copy Markdown
Member Author

@addaleax PTAL
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 24, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 24, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Aug 24, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@juanarbol juanarbol added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 24, 2023
@github-actions github-actions Bot mentioned this pull request Sep 4, 2023
CI Reliability 2023-09-04 nodejs/reliability#655
Open
22 tasks
@juanarbol juanarbol added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 4, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 4, 2023
@nodejs-github-bot

nodejs-github-bot commented Sep 4, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Sep 4, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Sep 5, 2023
CI Reliability 2023-09-05 nodejs/reliability#656
Open
19 tasks
@nodejs-github-bot

nodejs-github-bot commented Sep 5, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Sep 5, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Sep 6, 2023
CI Reliability 2023-09-06 nodejs/reliability#657
Open
25 tasks
@juanarbol juanarbol added the commit-queue PRs queued for automated landing through the Commit Queue. label Sep 6, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Sep 6, 2023
@nodejs-github-bot

nodejs-github-bot commented Sep 6, 2023

Copy link
Copy Markdown
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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@juanarbol

juanarbol commented Sep 6, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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
anonrig approved these changes Oct 22, 2023
View reviewed changes
@juanarbol juanarbol added commit-queue PRs queued for automated landing through the Commit Queue. and removed commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. labels Oct 22, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Oct 22, 2023
@nodejs-github-bot
nodejs-github-bot merged commit c206b2a into nodejs:main Oct 22, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 22, 2023

Copy link
Copy Markdown
Collaborator

Landed in c206b2a
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
targos pushed a commit that referenced this pull request Oct 23, 2023
@juanarbol @targos
worker: handle detached MessagePort from a different context
87be790 
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 <[email protected]>
PR-URL: #49150
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
@targos targos mentioned this pull request Oct 23, 2023
v21.1.0 release proposal #50335
Merged
targos pushed a commit that referenced this pull request Nov 11, 2023
@juanarbol @targos
worker: handle detached MessagePort from a different context
b4d149b 
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 <[email protected]>
PR-URL: #49150
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
@targos targos mentioned this pull request Nov 12, 2023
v20.10.0 release proposal #50682
Merged
@juanarbol
juanarbol deleted the juan/fix-worker-issue branch December 6, 2023 03:27
@juanarbol juanarbol mentioned this pull request Dec 6, 2023
2023-12-06, N|Solid v5.0.1 Iron Release nodesource/nsolid#41
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes
@anonrig anonrig anonrig approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

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 the worker_threads module and Worker API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Segmentation Fault on Contextifying MessagePort Transferred to a Worker Thread

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@juanarbol @nodejs-github-bot @addaleax @anonrig
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.