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: added NodeRuntime domain #27600

Closed
wants to merge 1 commit into from

Conversation

alexkozy
Copy link
Member

@alexkozy alexkozy commented May 7, 2019

Historically Node process sends Runtime.executionContextDestroyed
with main context as argument when it is finished.
This approach has some disadvantages. V8 prevents running some
protocol command on destroyed contexts, e.g. Runtime.evaluate
will return an error or Debugger.enable won't return a list of
scripts.
Both command might be useful for different tools, e.g. tool runs
Profiler.startPreciseCoverage and at the end of node process it
would like to get list of all scripts to match data to source code.
Or some tooling frontend would like to provide capabilities to run
commands in console when node process is finished to allow user to
inspect state of the program at exit.
This PR adds new domain: NodeRuntime. This domain sends
NodeRuntime.waitingForDisconnect when this event is enabled.

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 7, 2019 18:02
@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 7, 2019
@alexkozy alexkozy added the inspector Issues and PRs related to the V8 inspector protocol label May 7, 2019
src/inspector_agent.cc Outdated Show resolved Hide resolved
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!

src/inspector/node_protocol.pdl Outdated Show resolved Hide resolved
src/inspector/node_protocol.pdl Outdated Show resolved Hide resolved
src/inspector_agent.cc Outdated Show resolved Hide resolved
@@ -779,7 +797,8 @@ void Agent::WaitForDisconnect() {
}
// TODO(addaleax): Maybe this should use an at-exit hook for the Environment
// or something similar?
client_->contextDestroyed(parent_env_->context());
if (!client_->reportWaitingForDebuggerToDisconnect())
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe put this to NodeInspectorClient::contextDestroyed?

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'd like to have it here, otherwise in future someone might use contextDestroyed in other places and get misleading notification.

Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to rename "contextDestroyed" to something else - like "mainContextDone"?

src/inspector/runtime_agent.cc Outdated Show resolved Hide resolved
src/inspector/runtime_agent.cc Outdated Show resolved Hide resolved
@alexkozy alexkozy force-pushed the node-runtime-agent-2 branch 2 times, most recently from 2e3c872 to 051a90c Compare May 7, 2019 20:01
@alexkozy alexkozy requested a review from eugeneo May 8, 2019 00:13
experimental domain NodeRuntime
# When domain is enabled `NodeRuntime.waitingForDisconnect` will be issued at the end of node process.
# Please see `NodeRuntime.waitingForDisconnect` for more details.
command enable
Copy link
Contributor

Choose a reason for hiding this comment

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

"enable" seems to generic a verb for this very specific task. Maybe "retainMainContextForDebugger"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed to notifyWhenWaitingForDisconnect.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you forget to push the change?

Copy link
Member Author

Choose a reason for hiding this comment

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

it looks like something went wrong yesterday, I just pushed updated version.

src/inspector/node_protocol.pdl Show resolved Hide resolved
@@ -779,7 +797,8 @@ void Agent::WaitForDisconnect() {
}
// TODO(addaleax): Maybe this should use an at-exit hook for the Environment
// or something similar?
client_->contextDestroyed(parent_env_->context());
if (!client_->reportWaitingForDebuggerToDisconnect())
Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to rename "contextDestroyed" to something else - like "mainContextDone"?

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!

Copy link
Member Author

@alexkozy alexkozy left a comment

Choose a reason for hiding this comment

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

@eugeneo please take another look.

# Event is issued when Node process is finished and waiting for debugger to disconnect.
# When enabled client should listen for this event instead of `Runtime.executionContextDestroyed`
# to check wether node process is finished.
event waitingForDisconnect
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it needs to be communecated clearly on the purpose of this notification vs the Runtime.executionContextDestroyed - specifically, that the main context is retained and can still be used.

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 updated comment.

@@ -610,6 +620,14 @@ class NodeInspectorClient : public V8InspectorClient {
return false;
}

bool notifyWaitingForDisconnect() {
for (const auto& id_channel : channels_) {
if (id_channel.second->notifyWaitingForDisconnect())
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this notification will only get sent to the sessions that requested it. Others will get nothing, not even an Runtime.executionContextDestroyed.

I suggest sending other sessions executionContextDestroyed either as al else statement for this if or at a later point, once all sessions that requested a disconnect notification disconnect.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not possible to send executionContextDestroyed without dirty hacks. We need to intercept every Runtime.enable and Runtime.disable calls to maintain runtime enabled state on node side. At the same time we can not just calls executionContextDestroyed on inspector since then inspector will mark context as destroyed and any later calls that uses Runtime will return an error.

Based on how much hacks we need to add, how many clients there are in a wild right now, what is probability of using new client and old client at the same time I believe that we can just send nothing here. Protocol definition stated exactly this.

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 tried to adopt solution that you proposed and it seems like it works!

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!

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 13, 2019
@nodejs-github-bot
Copy link
Collaborator

@eugeneo
Copy link
Contributor

eugeneo commented May 13, 2019

Looks like this PR needs to be rebased by hand.

@Trott
Copy link
Member

Trott commented May 13, 2019

Looks like this PR needs to be rebased by hand.

If you are having CI difficulties, the actual problem is almost certainly the merge-commit. Our Jenkins setup often (always?) does not handle merge-commits well. Squash the merge-commit into the other commit or force-push the merge-commit out, and then CI should be fine.

@alexkozy alexkozy force-pushed the node-runtime-agent-2 branch 2 times, most recently from c7c45b4 to 0c5cb42 Compare May 13, 2019 23:43
@nodejs-github-bot
Copy link
Collaborator

Historically Node process sends Runtime.executionContextDestroyed
with main context as argument when it is finished.
This approach has some disadvantages. V8 prevents running some
protocol command on destroyed contexts, e.g. Runtime.evaluate
will return an error or Debugger.enable won't return a list of
scripts.
Both command might be useful for different tools, e.g. tool runs
Profiler.startPreciseCoverage and at the end of node process it
would like to get list of all scripts to match data to source code.
Or some tooling frontend would like to provide capabilities to run
commands in console when node process is finished to allow user to
inspect state of the program at exit.
This PR adds new domain: NodeRuntime. After
NodeRuntime.notifyWhenWaitingForDisconnect is enabled by at least one
client, node will send NodeRuntime.waitingForDebuggerToDisconnect
event instead of Runtime.executionContextDestroyed. Based on this
signal any protocol client can capture all required information and
then disconnect its session.
@nodejs-github-bot
Copy link
Collaborator

@alexkozy alexkozy self-assigned this May 14, 2019
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Test LGTM. Rest of it is RSLGTM/defer-to-@eugeneo-and-others.

@alexkozy
Copy link
Member Author

Landed in 10d7e01

git node land $PRID tool is amazing!

@alexkozy alexkozy closed this May 14, 2019
alexkozy added a commit that referenced this pull request May 14, 2019
Historically Node process sends Runtime.executionContextDestroyed
with main context as argument when it is finished.
This approach has some disadvantages. V8 prevents running some
protocol command on destroyed contexts, e.g. Runtime.evaluate
will return an error or Debugger.enable won't return a list of
scripts.
Both command might be useful for different tools, e.g. tool runs
Profiler.startPreciseCoverage and at the end of node process it
would like to get list of all scripts to match data to source code.
Or some tooling frontend would like to provide capabilities to run
commands in console when node process is finished to allow user to
inspect state of the program at exit.
This PR adds new domain: NodeRuntime. After
NodeRuntime.notifyWhenWaitingForDisconnect is enabled by at least one
client, node will send NodeRuntime.waitingForDebuggerToDisconnect
event instead of Runtime.executionContextDestroyed. Based on this
signal any protocol client can capture all required information and
then disconnect its session.

PR-URL: #27600
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request May 15, 2019
Historically Node process sends Runtime.executionContextDestroyed
with main context as argument when it is finished.
This approach has some disadvantages. V8 prevents running some
protocol command on destroyed contexts, e.g. Runtime.evaluate
will return an error or Debugger.enable won't return a list of
scripts.
Both command might be useful for different tools, e.g. tool runs
Profiler.startPreciseCoverage and at the end of node process it
would like to get list of all scripts to match data to source code.
Or some tooling frontend would like to provide capabilities to run
commands in console when node process is finished to allow user to
inspect state of the program at exit.
This PR adds new domain: NodeRuntime. After
NodeRuntime.notifyWhenWaitingForDisconnect is enabled by at least one
client, node will send NodeRuntime.waitingForDebuggerToDisconnect
event instead of Runtime.executionContextDestroyed. Based on this
signal any protocol client can capture all required information and
then disconnect its session.

PR-URL: #27600
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR mentioned this pull request May 21, 2019
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs 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++. inspector Issues and PRs related to the V8 inspector protocol 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.

6 participants