Skip to content

src: enter isolate before destructing IsolateData#51138

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.

Sign up for GitHub

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

Jump to bottom
Merged
nodejs-github-bot merged 1 commit into from
Dec 25, 2023
Merged

src: enter isolate before destructing IsolateData #51138

nodejs-github-bot merged 1 commit into from
Dec 25, 2023

Conversation

@bnoordhuis
Copy link
Member

@bnoordhuisbnoordhuis commented Dec 12, 2023

MVP fix for a worker_threads crash where ~WorkerThreadData() -> ~IsolateData() -> Isolate::DetachCppHeap() kicks off a round of garbage collection that expects an entered isolate.

No test because the crash is not reliably reproducable but the bug is pretty clearly described in the linked issue and is obvious once you see it, IMO.

Fixes: #51129

@bnoordhuis
src: enter isolate before destructing IsolateData
6bc6746
MVP fix for a worker_threads crash where ~WorkerThreadData() -> ~IsolateData() -> Isolate::DetachCppHeap() kicks off a round of garbage collection that expects an entered isolate. No test because the crash is not reliably reproducable but the bug is pretty clearly described in the linked issue and is obvious once you see it, IMO. Fixes: nodejs#51129
@nodejs-github-botnodejs-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. worker Issues and PRs related to Worker support. labels Dec 12, 2023
@lpincalpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 13, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 13, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@targos
Copy link
Member

targos commented Dec 13, 2023

@nodejs/cpp-reviewers

@lpinca
Copy link
Member

lpinca commented Dec 13, 2023

@nodejs/workers

@joyeecheungjoyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 14, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 14, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@debadree25debadree25 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 15, 2023
@joyeecheung
Copy link
Member

joyeecheung commented Dec 16, 2023•
edited
Loading

By the way it just occurred to me - consider this non-blocking - why can we not just do this directly in the IsolateData destructor instead? That would correct this for the embedder-facing node::FreeIsolateData() as well.

@bnoordhuis
Copy link
MemberAuthor

bnoordhuis commented Dec 16, 2023

I considered that but I'm not 100% sure it won't cause deadlocks.

juanarbol
juanarbol approved these changes Dec 19, 2023
View reviewed changes
Copy link
Member

@juanarboljuanarbol left a comment

Choose a reason for hiding this comment

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

LGTM

@jasnelljasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 22, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 22, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 23, 2023

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 24, 2023

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 25, 2023

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

@lpincalpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 25, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 25, 2023
@nodejs-github-botnodejs-github-bot merged commit aa87b77 into nodejs:mainDec 25, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 25, 2023

Landed in aa87b77

RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
@bnoordhuis@RafaelGSS
src: enter isolate before destructing IsolateData
54dd978
MVP fix for a worker_threads crash where ~WorkerThreadData() -> ~IsolateData() -> Isolate::DetachCppHeap() kicks off a round of garbage collection that expects an entered isolate. No test because the crash is not reliably reproducable but the bug is pretty clearly described in the linked issue and is obvious once you see it, IMO. Fixes: #51129 PR-URL: #51138 Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
@RafaelGSSRafaelGSS mentioned this pull request Jan 2, 2024
Merged
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@bnoordhuis@richardlau
src: enter isolate before destructing IsolateData
d76e16b
MVP fix for a worker_threads crash where ~WorkerThreadData() -> ~IsolateData() -> Isolate::DetachCppHeap() kicks off a round of garbage collection that expects an entered isolate. No test because the crash is not reliably reproducable but the bug is pretty clearly described in the linked issue and is obvious once you see it, IMO. Fixes: #51129 PR-URL: #51138 Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
@richardlaurichardlau mentioned this pull request Mar 25, 2024
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@QardQardQard approved these changes

@santigimenosantigimenosantigimeno approved these changes

@benjamingrbenjamingrbenjamingr approved these changes

@tniessentniessentniessen approved these changes

@joyeecheungjoyeecheungjoyeecheung approved these changes

@UlisesGasconUlisesGasconUlisesGascon approved these changes

@legendecaslegendecaslegendecas approved these changes

@juanarboljuanarboljuanarbol approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.c++Issues and PRs that require attention from people who are familiar with C++.needs-ciPRs that need a full CI run.workerIssues and PRs related to Worker support.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

worker_threads: race and crash on worker exit

14 participants

@bnoordhuis@nodejs-github-bot@targos@lpinca@joyeecheung@Qard@santigimeno@benjamingr@tniessen@UlisesGascon@legendecas@juanarbol@jasnell@debadree25