debugger: Reduce global variables and expose some APIs for the debugger.

[deanm]

Checklist

• tests and code linting passes
• the commit message follows commit guidelines

Affected core subsystem(s)

Node embedding API / startup / command line handling.

Description of change

The debugger setup is a bit complicated because of all of the cross signal handler / thread / global state. While I didn't completely remove all of the global state, I think this patch at least improves it and takes a step towards a cleaner API. This is also exposes a few additional functions required to make the debugger work from an embedder of Node that does its own startup (not via node::Start).

Reduce the amount of global state relating to starting the debugger
to just the port number, which is needed by the SIGUSR1 handler.

Expose APIs so that embedders of Node can also have some control over
the debugger setup process. SetMainIsolate is required to set the
global Isolate variable that the debugger related handlers use.
Expose StartDebug and EnableDebug for controlling the debug agent.

Remove the debug agent flag from NodeInstanceData and instead track via a
separate DebugSettings struct and plumb that through as much as possible to
remove the majority of reliance on global settings.

[reshnm]

I like this one.
It would also be nice to have an API to stop the debugger.

[deanm]

@jasnell @indutny Mind having a look? Thanks!

[addaleax]

nit: I’d like to see a comment here saying what the return value means.

[addaleax]

LGTM with a few nits

[jasnell]

@deanm ... not my area of expertise. /cc @bnoordhuis

[deanm]

Thanks @addaleax for the comments, updated to address all of them.

[addaleax]

Thanks! @deanm You should try to keep your commit message formatted as mentioned in the guidlines, i.e. <= 50 characters for the first line and <= 72 characters for the subsequent ones.

And agreed, it would be nice to have someone comment on this who is more familiar with the code here. It only looks good to me because it’s just moving code around to reduce global state, which is almost always a good idea.

[addaleax]

@deanm Sorry for the delays, but this needs to be rebased against master.

[bnoordhuis]

No need to qualify, v8::Isolate is imported at the top.

[bnoordhuis]

I personally don't think it's a good idea to expose debugger details in src/node.h. The implementation is probably going to change quite a bit when multi-isolate support is added.

[deanm]

@bnoordhuis Do you have a suggestion for an alternative approach? Right now there are not so many details exposed, just start/stop, etc. Currently there is not a way to get the debugger working when embedding node.

[bnoordhuis]

@deanm I think your best best for now is to float this patch. I'll be working on multi-isolate support this development cycle and I hope to get it into v7. I'll keep the embedder + debugger use case in mind.

[jkrems]

Linking to node-inspect issue because this PR might be affected by the potential removal of the debugger code.

nodejs/diagnostics#67

[jasnell]

Is this something we'd do at all now given that the old debugger is being removed / deprecated?

[deanm]

I haven't had a chance to follow the debugger related changes, but saw that Node now integrates with Chrome's dev tools. Sorry for asking such a basic question, but is there sort of an overview discussion somewhere about all of the changes, and what that means for embedding Node?

[addaleax]

@deanm It’s probably best to raise your questions/concerns at the https://github.com/nodejs/diagnostics issue tracker, I’d guess?

[deanm]

I had a look at some of the changes around the new inspect tools. I don't think that really eliminates the need for what this patch was trying to do. From what I can tell, even with the new inspect debugger functionality, you still have a similar process of parsing the command line arguments, decide if/when/how to start the debugger, etc.

For embedders of node, this functionality should still be exposed, there is still the process of startup (StartDebug, EnableDebug), shutdown (WaitForInspectorDisconnect), etc, which is currently not available outside of node's internal Start in node.cc.

When I wrote the original Node embedding APIs (f67e8f2), the intention was to allow an embedder the ability to write their own Start, meaning to run their own event loop (as it might need to integrate it with other things), etc.

An alternative would be to basically take the do/while event pump loop out of Start() and into a function that is passed in to start, allowing an embedder to supply an alternate main pump loop. I think either approach would be okay from my perspective, but it is essential that somehow embedding applications are able to implement a custom event loop.

I can draft up an example of passing a pump function through Start()...

[bnoordhuis]

Making the event loop driver/pump configurable sounds like a decent idea.

[jasnell]

should this remain open?

[bnoordhuis]

The debugger changes probably can't land since we're phasing it out. Reshuffling to make embedding easier is still acceptable and a good idea but perhaps we can move that to a separate PR.

[Trott]

@deanm Should this remain open? Are you going to try to rework it for current Node.js? Can you rebase? Or is this something you're unlikely to come back to?

[bnoordhuis]

I'll close this out because the debugger has been removed.

@deanm A PR to ease embedding is still welcome.