-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
src: fix Worker termination when --inspect-brk
is passed
#53724
src: fix Worker termination when --inspect-brk
is passed
#53724
Conversation
Review requested:
|
/cc @nodejs/cpp-reviewers @nodejs/inspector @nodejs/workers |
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
ba88cf2
to
264d531
Compare
This comment was marked as outdated.
This comment was marked as outdated.
src/node.cc
Outdated
void Environment::WaitForInspectorFrontend() { | ||
if (!inspector_agent_->WaitForConnectByOptions()) { | ||
return; | ||
} | ||
|
||
if (inspector_agent_->options().break_node_first_line) { | ||
inspector_agent_->PauseOnNextJavascriptStatement("Break at bootstrap"); |
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.
When I first looked at the function name, I expected it to only wait for the frontend connection. But, it also sets a breakpoint at the first line if specified in the options. Does it make sense for this function to handle setting the breakpoint as well?
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.
The function name omitted words like 'based on options set at creation', which I initially thought was okay. I renamed it to make it clearer. PTAL.
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.
lgtm
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
This comment was marked as outdated.
This comment was marked as outdated.
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.
lgtm
Landed in e1b80a5 |
Fixes: #53648
When calling
node::CreateEnvironment
with debug options like--inspect-brk
, Environment creation is blocked until an Inspector frontend connects. While creating a Worker thread's Environment, its InspectorAgent, which is owned by the Environment instance and controls exit, is not usable until creation is complete.This patch allows separate handling of Environment creation and Inspector frontend connection waiting, resolving the issue by creating the Environment instance first in worker threads.
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com