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.

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

inspector: add way to force worker to wait inspector session detach #27677

Closed
alexkozy opened this issue May 13, 2019 · 2 comments
Closed

inspector: add way to force worker to wait inspector session detach #27677

alexkozy opened this issue May 13, 2019 · 2 comments

Comments

@alexkozy
Copy link
Member

alexkozy commented May 13, 2019 •
edited
Loading

Is your feature request related to a problem? Please describe.
I am working on a tool that exposes different information available over the inspector protocol from Node (https://github.com/ak239/thetool). Right now I can get the data from processes and I would like to add way to get this data from workers as well.

With regular process following way to get all information is available:

  1. Start process with --inspect-brk flag in this case process will wait for Runtime.runIfWaitingForDebugger call.
  2. Start tool, tool connects to process, prepare everything, e.g. enable profiler using Profiler.start and when ready tool starts process using Runtime.runIfWaitingForDebugger.
  3. When process is finished it sends Runtime.executionContextDestroyed for main context and waits for tool to disconnect.
  4. Tool gets events capture all required data and disconnect.
  5. Node process finished.

In this case we have nice way to get all data and avoid any possible race condition.

For workers right now:

  1. Tool calls NodeWorker.enable with waitForDebuggerOnStart equals true.
  2. When worker is started, it sends NodeWorker.attachedToWorker and waits tool to call Runtime.runIfWaitingForDebugger.
  3. Tool prepare session, e.g. start profiler and send Runtime.runIfWaitingForDebugger
  4. Worker sends NodeWorker.detachedFromWorker. When tool gets this event it is too late to try to get any data from worker session. It is gone.

Describe the solution you'd like
Based on @eugeneo comment:
Implement NodeRuntime in worker. NodeRuntime. notifyWhenWaitingForDisconnect will force Worker to wait for disconnect and worker will send NodeRuntime. waitingForDisconnect when it is done. NodeWorker domain should get NodeWorker.detach to make disconnect possible.

@eugeneo what do you think?
@eugeneo
Copy link
Contributor

eugeneo commented May 13, 2019

Can this be joined with the #27600 - e.g. sending a NodeRuntime. notifyWhenWaitingForDisconnect to a worker will keep it alive while the frontend needs it.

@alexkozy alexkozy mentioned this issue May 15, 2019
Closed
3 tasks
alexkozy added a commit to alexkozy/node that referenced this issue May 15, 2019
@alexkozy @alexeykozy
inspector: supported NodeRuntime domain in worker
babdf80 
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
pull bot pushed a commit to daddyfatstacksBIG/node that referenced this issue Jun 2, 2019
@alexkozy
inspector: supported NodeRuntime domain in worker
7e18c65 
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 issue Jun 3, 2019
@alexkozy @targos
inspector: supported NodeRuntime domain in worker
23119ca 
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>
@alexkozy
Copy link
Member Author

alexkozy commented Jul 3, 2019

Fixed.

@alexkozy alexkozy closed this as completed Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@eugeneo @alexkozy