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: introduce inspector.SyncSession #27110

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

This patch introduces a synchronous inspector session that
dispatches the message and returns the result synchronously
to the user.

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

This patch introduces a synchronous inspector session that
dispatches the message and returns the result synchronously
to the user.
@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++. inspector Issues and PRs related to the V8 inspector protocol util Issues and PRs related to the built-in util module. labels Apr 6, 2019
@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 6, 2019
@addaleax
Copy link
Member

addaleax commented Apr 6, 2019

Is the main difference to regular sessions that the result is returned from .post()?

@joyeecheung
Copy link
Member Author

@addaleax Yeah, that makes the invocations that intend to use the results synchronously looks less awkward. I'll still need to investigate if this make sense for most methods, though.

It seems inevitable that this has to be an EventEmitter to send out messages/notifications that are not triggered by a particular .post, I think that's probably the best abstraction we could end up with for these anyway?

I'll need to complete the docs before sending it out for review..

@targos
Copy link
Member

targos commented Apr 6, 2019

Isn't it enough to add a postSync wrapper method to the existing class? I don't think we need to optimize for performance here.

@eugeneo
Copy link
Contributor

eugeneo commented Apr 6, 2019

Chrome team considered the fact that all V8 inspector messages are processed synchronously an implementation detail and there was specific goal not to make it a part of the interface. Chrome already has some asynchronous inspector calls outside the V8 domains.

Promisifying this interface would be the correct way but so far it had not been possible due to the inspector restrictions (event loop may be paused while the messages are being processed)

I am concerned that this interface will be broken down the line, as JavaScript evolves or other domains are being added to the Node Inspector.

@joyeecheung
Copy link
Member Author

Isn't it enough to add a postSync wrapper method to the existing class? I don't think we need to optimize for performance here.

@targos Note that in the implementation, the sync variant uses Function::Call instead of MakeCallback to avoid triggering any microtasks or tick callbacks, or emitting any async hooks before returning the result to the user (which seems counterintuitive for a synchronous session). It's easier to implement this on a per-session basis, because the onMessage callback that connects the C++ and the JS land is per-session.

@joyeecheung
Copy link
Member Author

joyeecheung commented Apr 7, 2019

Chrome team considered the fact that all V8 inspector messages are processed synchronously an implementation detail and there was specific goal not to make it a part of the interface. Chrome already has some asynchronous inspector calls outside the V8 domains.

Does this apply to all commands? There are commands explicitly marked as async in the protocol configuration, so I assumed that commands that are not marked are synchronous. We could also throw errors for those commands in the synchronous session, when we end up having to implement those asynchronously.

I am concerned that this interface will be broken down the line, as JavaScript evolves or other domains are being added to the Node Inspector.

Would changes like that affect existing commands, or would they be added to the protocol as new commands? I believe if the commands have to return the result asynchronously, they would not look the same as they are now. Take heap snapshots for instance, even if you dispatch HeapProfiler.takeHeapSnapshot synchronously, you would not have the result immediately but instead have to listen to HeapProfiler.addHeapSnapshotChunk. So even if heap snapshots can be returned synchronously, you can force the users to handle them asynchronously by how you design the commands. If a result of a command has to be returned asynchronously, the command can be designed not to return the result immediately but instead deliver the result in events (I believe Debugger.pause / Debugger.paused also work that way).

If we end up having to return results of commands asynchronously in their own invocation, would it be possible to keep them separate from the synchronous ones, so a synchronous session still works for commands that don't have to be asynchronous? (from what I can tell from the templates, the async commands and the synchronous commands already have their code generated differently, it just happens that the async ones are also implemented synchronously in Node.js).
The commands themselves come from the inspector protocol, so the stability guarantee of that is out of the scope of Node.js and the users should expect changes there anyway - we could also make that clear in the docs.

@addaleax
Copy link
Member

addaleax commented Apr 7, 2019

Fwiw, takeHeapSnapshot is synchronous as well, and I think it would actually be difficult to implement it asynchronously?

@eugeneo
Copy link
Contributor

eugeneo commented Apr 7, 2019

Does this apply to all commands? There are commands explicitly marked as async in the protocol configuration, so I assumed that commands that are not marked are synchronous. We could also throw errors for those commands in the synchronous session, when we end up having to implement those asynchronously.

Yes, all commands are async. I am not sure what you mean by "protocol configuration" - if that is inspector_protocol_config.json then it is not a spec but a code generator configuration file, telling the code generator to pass in the callback instead of expecting a synchronous return value. In some cases in Chrome we flipped that switch as a part of an optimization or a refactoring effort and that did not require any change on the frontends. Note that the protocol viewer does not differentiate "async" and "sync" calls.

Also, I believe it will be hugely confusing if some messages would work through this API and some would not.

Fwiw, takeHeapSnapshot is synchronous as well, and I think it would actually be difficult to implement it asynchronously?

All messages are considered async, no message is supposed to be safe from becoming async...

@joyeecheung
Copy link
Member Author

joyeecheung commented Apr 7, 2019

For context, this came out of the discussion in https://chromium-review.googlesource.com/c/v8/v8/+/1546559

If we can't rely on the premise that we are able to use the inspector synchronously (wether exposed as a public API or not, at least for the commands we need), then we cannot rely on inspector in util.inspect(), and either need to implement embedder APIs for what we need, or only use the inspector in console (with functionality mismatch in util.inspect()) , though that'll transitively make the TTY console async, which could be breaking (but I don't know about the potential breakage...it's hard to imagine a hard dependency on console.log being synchronous, but it may introduce some ordering differences in existing code that confuse people)

@@ -1,7 +1,7 @@
'use strict';
Copy link
Member

Choose a reason for hiding this comment

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

Should we add docs for this 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.

I won't be writing docs until I confirm this feature actually makes sense..
(now I am leaning towards no)

@targos
Copy link
Member

targos commented Apr 8, 2019

What do you think about making this an internal feature only (for use in inspect an maybe tests)?

@joyeecheung
Copy link
Member Author

joyeecheung commented Apr 10, 2019

What do you think about making this an internal feature only (for use in inspect an maybe tests)?

If the synchronicity can be broken, then we can't really build features that depend on this, right?

@joyeecheung
Copy link
Member Author

joyeecheung commented May 7, 2019

I am going to use this PR (switched to internal as mentioned in #27110 (comment)) for the basis of an inspector-protocol based version of private fields inspection (#27404) given the feedback we've got from V8.

TL;DR: the suggestion was to build on top of the current synchronicity of the inspector implementation in util.inspect for now. Should the circumstances change we'll revisit whether it's better to expose APIs in V8 to work around any breakage.

@joyeecheung joyeecheung added the stalled Issues and PRs that are stalled. label Aug 24, 2019
@joyeecheung
Copy link
Member Author

I think a better plan for now is to publish this as an npm package instead of bringing it into core

@gengjiawen
Copy link
Member

I think a better plan for now is to publish this as an npm package instead of bringing it into core

publish it on npm may not bring enough attention.

@antsmartian
Copy link
Contributor

@joyeecheung Is this something we wanted to revisit again or as mentioned in comments, this shouldn't be in the core but to be part of a npm package?

@joyeecheung
Copy link
Member Author

@antsmartian yeah, I don’t think this should be in core at the moment.

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++. inspector Issues and PRs related to the V8 inspector protocol semver-minor PRs that contain new features and should be released in the next minor version. stalled Issues and PRs that are stalled. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants