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

inspector: supported NodeRuntime domain in worker #27706

Closed
wants to merge 1 commit into from

Conversation

alexkozy
Copy link
Member

@alexkozy alexkozy commented May 15, 2019

NodeRuntime domain was introduced to give inspector client way to
fetch captured information before Node process is gone. We need
similar capability for worker.

With current protocol inspector client can force worker to wait
on start by passing waitForDebuggerOnStart flag to NodeWorker.enable
method. So client has some time to setup environment, e.g. start
profiler. At the same time there is no way to prevent worker from
being terminated. So we can start capturing profile but we can not
reliably get captured data back.

This PR implemented NodeRuntime.notifyWhenWaitingForDisconnect
method for worker. When NodeRuntime.waitingForDisconnect notification
is enabled, worker will wait for explicit NodeWorker.detach call.

With this PR worker tooling story is nicely aligned with main thread
tooling story. The only difference is that main thread by default is
waiting for disconnect but worker thread is not waiting.

Issue: #27677

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@alexkozy alexkozy requested a review from eugeneo May 15, 2019 01:21
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels May 15, 2019
Copy link
Contributor

@eugeneo eugeneo 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 working on this!

@@ -71,6 +71,11 @@ experimental domain NodeWorker
# Detaches from all running workers and disables attaching to new workers as they are started.
command disable

# Detached from worker with given sessionId.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should be either "a worker" or "the worker". Not sure which :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the since it sounds like there is the worker with given sessionId but I am open for any input from more proficient in English colleagues :)

@@ -71,6 +71,11 @@ experimental domain NodeWorker
# Detaches from all running workers and disables attaching to new workers as they are started.
command disable

# Detached from worker with given sessionId.
command detach
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need attach for symmetry? The problem I see is that we do not track worker separately from a session so introducing attach would definitely make it more confusing...

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case should we have a little bit crazy NodeRuntime.disconnect method that works for main and worker thread?
I don't know any use case for NodeWorker.attach. Since workers are not waiting for attach by default, this method does not add any value without waitForDebuggerOnStart flag. Could we add it later when we know use case better?

Copy link
Contributor

Choose a reason for hiding this comment

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

One potential problem I see with the detach is that the worker is kind of "lost" once session is detached. A frontend may detach from the worker early and then there will be no way of attaching again. IMHO, attach can be implemented later if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case should we have a little bit crazy NodeRuntime.disconnect method that works for main and worker thread?

Don't think this is necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Feel free to ping me when someone would ask about this feature!

@@ -115,6 +115,11 @@ DispatchResponse WorkerAgent::disable() {
return DispatchResponse::OK();
}

DispatchResponse WorkerAgent::detach(const String& sessionId) {
workers_->Detached(sessionId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need a detachedFromWorker notification in response to a frontend request?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes! It is how most domains (e.g. Target domain) in Chrome works. Notification means that worker session was detached for some reasons, one of the reasons might be explicit protocol request. It is easy to ignore this notification on frontend if needed.

src/inspector_agent.cc Outdated Show resolved Hide resolved
@alexkozy alexkozy force-pushed the worker-node-runtime-support branch from efa78cb to 2399bb8 Compare May 15, 2019 16:50
NodeRuntime domain was introduced to give inspector client way to
fetch captured information before Node process is gone. We need
similar capability for work.

With current protocol inspector client can force worker to wait
on start by passing waitForDebuggerOnStart flag to NodeWorker.enable
method. So client has some time to setup environment, e.g. start
profiler. At the same time there is no way to prevent worker from
being terminated. So we can start capturing profile but we can not
reliably get captured data back.

This PR implemented NodeRuntime.notifyWhenWaitingForDisconnect
method for worker. When NodeRuntime.waitingForDisconnect notification
is enabled, worker will wait for explicit NodeWorker.detach call.

With this PR worker tooling story is nicely aligned with main thread
tooling story. The only difference is that main thread by default is
waiting for disconnect but worker thread is not waiting.

Issue: nodejs#27677
@alexkozy alexkozy force-pushed the worker-node-runtime-support branch from 2399bb8 to babdf80 Compare May 15, 2019 17:03
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@alexkozy
Copy link
Member Author

alexkozy commented Jun 2, 2019

Landed in 7e18c65

@alexkozy alexkozy closed this Jun 2, 2019
pull bot pushed a commit to daddyfatstacksBIG/node that referenced this pull request Jun 2, 2019
NodeRuntime domain was introduced to give inspector client way to
fetch captured information before Node process is gone. We need
similar capability for work.

With current protocol inspector client can force worker to wait
on start by passing waitForDebuggerOnStart flag to NodeWorker.enable
method. So client has some time to setup environment, e.g. start
profiler. At the same time there is no way to prevent worker from
being terminated. So we can start capturing profile but we can not
reliably get captured data back.

This PR implemented NodeRuntime.notifyWhenWaitingForDisconnect
method for worker. When NodeRuntime.waitingForDisconnect notification
is enabled, worker will wait for explicit NodeWorker.detach call.

With this PR worker tooling story is nicely aligned with main thread
tooling story. The only difference is that main thread by default is
waiting for disconnect but worker thread is not waiting.

Issue: nodejs#27677

PR-URL: nodejs#27706
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
targos pushed a commit that referenced this pull request Jun 3, 2019
NodeRuntime domain was introduced to give inspector client way to
fetch captured information before Node process is gone. We need
similar capability for work.

With current protocol inspector client can force worker to wait
on start by passing waitForDebuggerOnStart flag to NodeWorker.enable
method. So client has some time to setup environment, e.g. start
profiler. At the same time there is no way to prevent worker from
being terminated. So we can start capturing profile but we can not
reliably get captured data back.

This PR implemented NodeRuntime.notifyWhenWaitingForDisconnect
method for worker. When NodeRuntime.waitingForDisconnect notification
is enabled, worker will wait for explicit NodeWorker.detach call.

With this PR worker tooling story is nicely aligned with main thread
tooling story. The only difference is that main thread by default is
waiting for disconnect but worker thread is not waiting.

Issue: #27677

PR-URL: #27706
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
@targos targos mentioned this pull request Jun 3, 2019
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++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants