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

Port the CLI debugger to use the v8 inspector protocol #7266

Closed
ofrobots opened this issue Jun 10, 2016 · 20 comments
Closed

Port the CLI debugger to use the v8 inspector protocol #7266

ofrobots opened this issue Jun 10, 2016 · 20 comments
Labels
feature request Issues that request new features to be added to Node.js. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. inspector Issues and PRs related to the V8 inspector protocol

Comments

@ofrobots
Copy link
Contributor

ofrobots commented Jun 10, 2016

Now that we have the v8_inspector available in Node, it would be good to create port of the CLI debugger that uses the new inspector protocol. This would have a few benefits:

  • It would make it easier to unit test inspector debug functionality for Node. UI based tests are not going to be easy to automate in the Node.js test infrastructure.
  • The CLI debugger could start taking advantage of the newer features available through the v8 inspector protocol.
  • We can stop depending on the deprecated V8 debug apis that the old protocol uses.
  • Over the long term we would like to converge back to a single debug server implementation and protocol. This would be a semver-major, so this shouldn't be done immediately, but maybe v8.x is a good target for this part?

Contributions to make this happen would be most welcome.

/cc @pavelfeldman @thealphanerd @nodejs/diagnostics

@ofrobots ofrobots added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. feature request Issues that request new features to be added to Node.js. inspector Issues and PRs related to the V8 inspector protocol labels Jun 10, 2016
@MylesBorins
Copy link
Contributor

very much into this idea!

There are quite a few edge cases in the current CLI debugger... I'm not very familiar with the problem space, would the port be fairly trivial or should we consider starting something from scratch (so we can make new and different bugs).

Where is the new protocol documented?

@MylesBorins
Copy link
Contributor

Also does this make sense to open in @nodejs/diagnostics to discuss implementation?

@targos
Copy link
Member

targos commented Jun 10, 2016

I'd love to help!

@eugeneo
Copy link
Contributor

eugeneo commented Jun 10, 2016

The main issue I see is that _debugger.js would need to support WebSocket protocol.

@ofrobots
Copy link
Contributor Author

@thealphanerd You can use the protocol viewer for the documentation here: https://chromedevtools.github.io/debugger-protocol-viewer/v8/. For background also see Chrome Debugger Protocol.

The main issue I see is that _debugger.js would need to support WebSocket protocol.

That is true. But since the inspector only uses a minimal subset of WebSockets, I think it should be possible to hack together a small client that is able to talk to the server.

@eugeneo
Copy link
Contributor

eugeneo commented Jun 10, 2016

inspector_socket.{h,cpp} only includes server socket implementation, e.g. it does not mask the frames. Also, it does not have JS bindings for the _debugger.js to use.

Of course, that implementation can be extended and the bindings can be introduced.

@joshgav
Copy link
Contributor

joshgav commented Jun 10, 2016

What do you think of extending CodeGenerator.py to generate a client proxy too? That could be an ideal foundation for the CLI debugger and other clients.

Also finally got to read all the code today :) and have been thinking we might separate:

  • v8_inspector/platform/inspector_protocol/*
  • v8_inspector/devtools/* and
  • v8_inspector/platform/v8_inspector/public/V8Inspector.[h|cpp]

into a separate "Inspector Protocol Gateway" project. That gateway could dynamically load agents like those listed here, and similar to what V8Inspector.cpp does now here, perhaps by looking in a designated folder or a manifest file.

Could be a way to build an ecosystem of domains and agents around this protocol?

Was thinking of working on that next week...

@pavelfeldman
Copy link
Contributor

What do you think of extending CodeGenerator.py to generate a client proxy too?

I don't think we should unify to make it a part of CodeGenerator.py. We use a separate 270 lines generator for the DevTools front-end and since the format is well-specified, generator has no maintenance cost. You'll want to generate node-friendly call conventions, use promises, etc.

into a separate "Inspector Protocol Gateway" project

Yes, that sounds like a good direction in general. Some thoughts on that:

platform/inspector_protocol is supposed to be reusable codegen infrastructure for the protocol. You should be able to use it to generate native bindings and wired api for given protocol definition. We use it for v8_inspector and for chromium. It is unclear whether it'll be exposed via v8 API though once we migrate the code there, so we might want to keep it as a third party separately. Not sure yet.

platform/v8_inspector uses the framework above to handle JS-related domains. You wire it to the socket and it does the job. Any other component, including Node could do the same, inspector_protocol should cover all the infrastructure needs. I already extracted js_protocol.json from the giant protocol file and it now belongs to platform/v8_inspector, v8_inspector/devtools no longer exists. We'll roll it along with more fixes soon.

V8Inspector.[h|cpp] is now empty. But you do want a wrapper around the session that would handle the Node-level domains. At this point, inspector_agent is the best place for it.

@ofrobots
Copy link
Contributor Author

I already extracted js_protocol.json from the giant protocol file and it now belongs to platform/v8_inspector, v8_inspector/devtools no longer exists. We'll roll it along with more fixes soon.

In #7302.

@joshgav
Copy link
Contributor

joshgav commented Jun 15, 2016

@pavelfeldman @ofrobots Thanks for the background and the new roll. I'm still planning to work on this soon. Pavel, what you said makes sense and I think we're on the same page; I plan to prototype out the separation somewhere and we can go from there.

@ofrobots
Copy link
Contributor Author

ofrobots commented Sep 8, 2016

#8429 could be as an example / starting point for the CLI debugger port.

@jkrems
Copy link
Contributor

jkrems commented Sep 8, 2016

Awkward, didn't see this issue when I searched for it. I have a mostly working port here: nodejs/node-inspect#1

@jkrems
Copy link
Contributor

jkrems commented Sep 13, 2016

After doing a (more or less) complete implementation of the current debugger interface on top of the new protocol: It's a bit awkward. E.g. given that we now have the inspector command line API and proper remote object support, a "remote repl first" approach (without switching between two modes) using .cmd for step actions etc. might feel a lot nicer.

@MylesBorins
Copy link
Contributor

Where are we at on this?

@ofrobots
Copy link
Contributor Author

@jkrems sorry for the late response. It is awesome to see your progress here!

Can you elaborate a bit more on what you consider to be the 'awkward' bits? There a few use-cases, where a CLI based debugger is really useful, e.g. debugging a VM in a production/restricted environment where it isn't possible to expose ports to allow a remote debugger to attach.

Would you be willing to contribute your CLI debugger into node-core?

@jkrems
Copy link
Contributor

jkrems commented Sep 30, 2016

@ofrobots Sure, to elaborate: I didn't mean "a command line debugger is awkward" but "the current command line debugger user interface is awkward". I suspect it might be a better dev experience if we would default to "it works like the chrome devtools console tab + .bt/.n/... commands for the buttons". So "remote repl first" as opposed to a local repl with magical properties and methods on the context that can turn into a remote repl but loses all debug control commands while the remote repl is active.

There's still some cleanup to be done on the code I linked to (although it should be ~95% there). It should be pretty straight-forward since it's basically a copy of the existing debugger file and has no external dependencies. The main reason I'm hesitant to do it is that I'm not fully convinced that I'd like the end result. ;)

@jkrems
Copy link
Contributor

jkrems commented Oct 10, 2016

Follow-up: This was discussed in the last diagnostics meeting and it was suggested to move node-inspect into the nodejs org. This work is tracked here: nodejs/diagnostics#67.

@joshgav
Copy link
Contributor

joshgav commented Jan 23, 2017

node-inspect is now at https://github.com/nodejs/node-inspect. Thank you @jkrems!

@jkrems
Copy link
Contributor

jkrems commented Mar 29, 2017

Should we close this issue?

@ofrobots
Copy link
Contributor Author

ofrobots commented Apr 6, 2017

I don't see any reason to keep this open given that we have have a new CLI debugger available 7.x already, and #11421 tracks the migration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants