Skip to content

No call to SetIdle() when terminating - #45422

Closed
ywave620 wants to merge 5 commits into
nodejs:mainfrom
ywave620:no-SetIdle-when-terminating
Closed

No call to SetIdle() when terminating#45422
ywave620 wants to merge 5 commits into
nodejs:mainfrom
ywave620:no-SetIdle-when-terminating

Conversation

@ywave620

@ywave620ywave620 commented Nov 11, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Fix#45421, where you can find a helpful stack trace

One can argues that v8 is responsable for transfering vm state to EXTERNAL before invoking PerIsolateMessageListener(), if v8 agrees and changes for it, this issue is gone without any change of Node.js. However, SetIdle() is basically for cpu profiling only, it's not necessary when terminating. Removing it is not harmful while otherwise will lead to crash before v8 makes that change.

@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. labels Nov 11, 2022
@ywave620ywave620 changed the title No set idle when terminatingNo call to SetIdle() when terminatingNov 11, 2022
@ywave620
src: no call to SetIdle() when terminating
c0e3840
Calling SetIdle() when terminating is not harmless.
When node terminates due to an unhandled exception,
v8 preseves the vm state, which is JS and notifies
node through PerIsolateMessageListener(). If node
calls SetIdle() later, v8 complains because it
requires the vm state to either be EXTERNEL or IDLE
when embedder calling SetIdle().
@ywave620
ywave620force-pushed the no-SetIdle-when-terminating branch from 24958ca to c0e3840CompareNovember 11, 2022 02:42
@ywave620

ywave620 commented Nov 11, 2022

Copy link
Copy Markdown
ContributorAuthor

Welcome to join discussion about v8's behavior in https://bugs.chromium.org/p/v8/issues/detail?id=13464

bnoordhuis
bnoordhuis approved these changes Nov 12, 2022
View reviewed changes

@bnoordhuisbnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with the caveat that this may end up swapping one bug for another, although it's probably a hypothetical scenario.

I'm thinking of when someone calls isolate->TerminateExecution(), then later on calls isolate->CancelTerminateExecution().

Comment threadtest/parallel/test-unhandled-exception-with-worker-inuse.js Outdated
@ywave620@bnoordhuis
src: no call to SetIdle() when terminating
82f26a2
Fix typo
Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl>
@bnoordhuisbnoordhuis added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Nov 15, 2022
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 15, 2022
@nodejs-github-bot

nodejs-github-bot commented Nov 15, 2022

Copy link
Copy Markdown
Collaborator

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

legendecas
legendecas reviewed Nov 15, 2022
View reviewed changes
Comment threadtest/parallel/test-unhandled-exception-with-worker-inuse.js Outdated
@ywave620
src: no call to SetIdle() when terminating
6bb4c86
use fixture function to check no abort
@legendecaslegendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 16, 2022
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 16, 2022
@nodejs-github-bot

nodejs-github-bot commented Nov 16, 2022

Copy link
Copy Markdown
Collaborator

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

@ywave620

ywave620 commented Nov 16, 2022

Copy link
Copy Markdown
ContributorAuthor

Sry, the previous commits forgets to wrap an asserttion around common.nodeProcessAborted. Fixed in the latest one. Please rerun the CI.

@legendecaslegendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 16, 2022
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 16, 2022
@nodejs-github-bot

nodejs-github-bot commented Nov 16, 2022

Copy link
Copy Markdown
Collaborator

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

This was referenced Nov 17, 2022
Open
Open
joyeecheung
joyeecheung approved these changes Nov 18, 2022
View reviewed changes

@joyeecheungjoyeecheung left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the changes here make sense with or without a fix in V8 to update the state to EXTERNAL before invoking the message listener (I think that should also be done - SetIdle() might not be the only victim of it)

santigimeno
santigimeno approved these changes Nov 18, 2022
View reviewed changes

@santigimenosantigimeno left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. By the way, this will fix #41107

@github-actionsgithub-actionsBot mentioned this pull request Nov 19, 2022
Open
10 tasks
This was referenced Nov 20, 2022
Open
Open
@nodejs-github-bot

nodejs-github-bot commented Nov 21, 2022

Copy link
Copy Markdown
Collaborator

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

This was referenced Nov 22, 2022
Open
Open
@ywave620ywave620 mentioned this pull request Nov 23, 2022
Merged
@ywave620

ywave620 commented Nov 23, 2022

Copy link
Copy Markdown
ContributorAuthor

I have moved this change to #45596, which includes a fix to another bug about terminating. PTAL.

@github-actionsgithub-actionsBot mentioned this pull request Nov 24, 2022
Open
16 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuisbnoordhuisbnoordhuis approved these changes
@jasnelljasnelljasnell approved these changes
@santigimenosantigimenosantigimeno approved these changes
@joyeecheungjoyeecheungjoyeecheung approved these changes
@legendecaslegendecaslegendecas approved these changes
@theanarkhtheanarkhtheanarkh approved these changes

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Crash if unhandled exception is thrown inside a node program with worker_threads inuse

8 participants

@ywave620@nodejs-github-bot@bnoordhuis@jasnell@santigimeno@joyeecheung@legendecas@theanarkh