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

Debug already running process using v8-inspector? #8464

Closed
roblourens opened this issue Sep 9, 2016 · 52 comments
Closed

Debug already running process using v8-inspector? #8464

roblourens opened this issue Sep 9, 2016 · 52 comments
Assignees
Labels
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 question Issues that look for answers.

Comments

@roblourens
Copy link

Currently you can send SIGUSR1 to a Node process to put it into debug mode as if it had been launched with --debug. Docs here. Is there any way to do the same using the new v8-inspector protocol?

@mscdex mscdex added question Issues that look for answers. debugger inspector Issues and PRs related to the V8 inspector protocol labels Sep 9, 2016
@joshgav
Copy link
Contributor

joshgav commented Sep 12, 2016

Perhaps we should add SIGUSR2 to activate inspector and leave SIGUSR1 to activate the old protocol?

Working impl of this here: joshgav@0edde39

/cc @nodejs/diagnostics

@ofrobots
Copy link
Contributor

ofrobots commented Sep 12, 2016

The problem is that there aren't enough signals, and SIGUSR2 might already be in use by user-space modules. Repurposing SIGUSR2 would be a semver major change.

I think a better approach would be to deprecate the old debugger in v8.x and free up SIGUSR1 for use by the inspector based protocol. Unfortunately this means that we won't have a solution to attach inspector to an already running process for v7.x and v6.x.

@papandreou
Copy link
Contributor

SIGUSR2 is used by https://github.com/bnoordhuis/node-heapdump (triggers a heap dump).

@joshgav
Copy link
Contributor

joshgav commented Sep 13, 2016

Is there a reasonable way to parameterize signals so we could double up SIGUSR1? One approach could be a config file next to the node binary.

/cc @bnoordhuis re node-heapdump. Do we know of other modules which use SIGUSR2?

@ofrobots:

we won't have a solution to attach inspector to an already running process for v7.x and v6.x

IMHO this would be a significant problem for inspector protocol adoption and Node diagnostics in general. We can't expect tools to use a different protocol for attaching to a running process, which is an important use case.

Waiting till v8.x (summer 2017) is too long.

deprecate the old debugger in v8.x and free up SIGUSR1

Even if/when we deprecate the old protocol I don't think we can change behavior of SIGUSR1 for another release or several. Tools will continue to support the old protocol, and v4.x will still be around till Oct 2017 at least.

@bnoordhuis
Copy link
Member

The inspector using SIGUSR2 is not a problem for node-heapdump, it will simply override the inspector's signal handler. That may be a problem for the inspector, though. :-)

@ofrobots
Copy link
Contributor

Is there a reasonable way to parameterize signals so we could double up SIGUSR1? One approach could be a config file next to the node binary.

This is not a bad idea. Another idea would be a flag, e.g. --inspect-on-sigusr1. /cc @nodejs/v8-inspector.

@roblourens
Copy link
Author

I like the config file option, in that if there is a flag to set when you start node, people will inevitably forget to set that flag, then if you have to restart the process anyway you might as well start it in debug mode. I don't know whether there's some typical way to handle this kind of scenario though.

@joshgav
Copy link
Contributor

joshgav commented Sep 13, 2016

Right, seems that if we could go back and restart with a flag we could just use --inspect itself. We need a way to notify and change a running process, which suggests a signal. Any alternatives to signals?

Assuming we have to use a signal, it seems we'd prefer to reuse SIGUSR1. Even if we accepted the semver-major aspect of using SIGUSR2, I think taking the only other user signal would be inconsiderate to @bnoordhuis and others. (I didn't realize earlier that there are only 2! 😮 )

However, since SIGUSR1 is already in use in Node we need to parameterize it somehow so Node knows whether to start the old debugger agent or the new inspector agent. I suggested a config file and will throw together a patch to demonstrate. Maybe one day when the old debugger is gone we can remove it. Are there other patterns to somehow parameterize signals?

@bnoordhuis
Copy link
Member

I don't like the idea of a config file. Node has always been a self-contained binary and it should stay that way.

@ofrobots
Copy link
Contributor

Config files don't have a precedent, and quite a few issues will need to be worked through: does the config belong in the node installation, or does it belong with an application? Where do you find the config relative to the application? Should it be something in the package.json? What about standalone scripts, etc.

Other options:

  • An environment variable.
  • A named pipe that Node listens on.

@imyller
Copy link
Member

imyller commented Sep 14, 2016

+1 for --inspect-on-sigusr1 flag along with support for matching environment variable (e.g. NODE_INSPECT_ON_SIGUSR1).

Flag alone would be handy but sometimes you want to use exported environment variable to enable some behaviour in child processes too.

Flag wouldn't be semver-major change, right?

@bnoordhuis
Copy link
Member

I'd say semver-minor, it's additive.

@joshgav
Copy link
Contributor

joshgav commented Sep 14, 2016

@bnoordhuis @ofrobots Agreed, I also don't like the config file, I think it's too complex for the problem at hand.

But env vars or flags have to be set before the process is run, and we need something to modify a running process. Am I perhaps missing something?

@ofrobots - Named pipe is interesting, would that completely replace the signal?

Another idea - Perhaps we can add something to the existing debug agent which would allow us to ask it to switch to the inspector agent immediately? This would certainly be a more complex implementation, and there would be a slight perf penalty when a debug session starts, but that seems okay since it's intended for debug time and the tradeoff would be a better user experience in tools.

Also, once we're ready we can make the inspector agent the default and drop this (or more likely swap it to enable the old debugger agent).

@eugeneo
Copy link
Contributor

eugeneo commented Sep 14, 2016

Currently both inspector and debugger have their own server socket implementations. I could try to refactor that into a separate codebase that will start an appropriate agent based on whether the first massage is an HTTP request... I don't think it would be possible to have both agents running at the same time - so once the debugger or inspector is started, second implementation will never run.

@eugeneo
Copy link
Contributor

eugeneo commented Sep 14, 2016

Is there a roadmap for deprecating the debugger agent?

@joshgav
Copy link
Contributor

joshgav commented Sep 14, 2016

@eugeneo

start an appropriate agent based on whether...

This sounds good to me! Once doing it though it might be worth giving it more flexibility than just switching on the protocol itself. Perhaps you could include a simple way to parameterize that first request, could be just a query param in the URL?

Is there a roadmap for deprecating the debugger agent?

IMHO we should wait to even discuss till v8.x timeframe (i.e. summer 2017) to see what state of ecosystem is - e.g. tools, helper modules, etc.

So for purposes of this thread I think we should consider the debugger agent the default and inspector agent secondary.

@ofrobots
Copy link
Contributor

The old debug protocol in V8 has been deprecated for a number of years. Now that v8_inspector has just landed in V8 itself, I expect the old debug protocol is probably going to be removed late this year or early next year. I think we will need to have deal with the deprecation in the v8.x release.

@ofrobots
Copy link
Contributor

Another idea: if the ports are different, perhaps we can start both debug servers when SIGUSR1 arrives (and optionally close the other port one when the first port gets a connection.)

@auchenberg
Copy link

I like that approach @ofrobots. How do we move forward from here? This functionality will enable a much better user experience for 3rd party devtools.

@bnoordhuis
Copy link
Member

@auchenberg If you want to work on this, the logic is in DispatchDebugMessagesAsyncCallback and StartDebug/EnableDebug in src/node.cc.

@joshgav
Copy link
Contributor

joshgav commented Nov 23, 2016

@bnoordhuis @auchenberg let's make sure we have consensus on what behavior should in fact be implemented. In the last Diagnostics WG meeting (nodejs/diagnostics#74) we tentatively said we'd wait till the current debugger can be deprecated and then switch the USR1 signal to activate the inspector gateway instead. But it'll probably be a while till we can truly deprecate the old debugger, so an interim solution may be needed.

Following are the options suggested in this thread, in reverse order. I'll use the term "debugger" to refer to the legacy debugger agent, and "inspector" for the new one.

  1. Start both inspector and debugger on SIGUSR1, shut the unused one down when messages are received on the other.
    • Works because they listen on different ports by default (5858, 9229).
  2. Start a small intermediary gateway on SIGUSR1 which will activate the debugger or inspector depending on initial messages.
    • Could interfere with inspector protocol specification and HTTP calls to /json metadata.
  3. Listen on a pre-specified named pipe instead of a signal.
    • Message sent through pipe could be more parameterized and explicit than a signal.
  4. Configuration file.
    • Significant implications for Node in general which we might not want to deal with.
  5. Environment variable which indicates what SIGUSR1 should do.
    • Can this be injected into a running process?
  6. --inspect-on-sigusr1 runtime flag to switch SIGUSR1 behavior for process.
    • Requires restarting the process, in which case one could specify --inspect.
  7. SIGUSR2 activates inspector, SIGUSR1 activates debugger.
    • Incompatible with modules which already use SIGUSR2 for their own purposes.
    • Unfairly takes all user signals for Node.

I vote for [1] as it matches the current behavior quite closely and provides a smooth exit path when the debugger is deprecated.

@bnoordhuis
Copy link
Member

Yes, option 1 seems like the way forward and is in fact what I thought had been agreed upon previously.

@joshgav
Copy link
Contributor

joshgav commented Feb 1, 2017

@eugeneo

The main issue is that V8 inspector instance will have to always be created during Node startup.

At the surface this seems non-trivial. Do we have benchmarks showing diff between Node with Inspector activated and not? Are there other concerns to think about with having it always on?

console.log intercept

Would this be in place immediately, or simply ready to go when a signal is received?

@ofrobots
Copy link
Contributor

ofrobots commented Feb 1, 2017

The inspector being present is equivalent to being able to run const Debug = vm.runInDebugContext('Debug') today. I think it is important that no debug port is open unless the user explicitly asks for one, either via a command line option or by sending a signal (as today).

@joshgav
Copy link
Contributor

joshgav commented Feb 2, 2017

@ofrobots

no debug port is open

And to be explicit, the separate uv loop and thread would not be created until/unless a signal is received, right?

@eugeneo
Copy link
Contributor

eugeneo commented Feb 2, 2017

@joshgav
IO thread is not created until the signal is received. Note that there's still a separate signal handler thread (same as current debugger) - I considered reusing it for running the IO loop, but for now decided it will complicate the code without much gain.

@ofrobots
Copy link
Contributor

ofrobots commented Feb 2, 2017

Wholehearted +1 on not reusing the signal thread as the debug IO thread.

@ORESoftware
Copy link
Contributor

Hey all, I am curious if I can debug an already running node process with node --inspect, is there a way?

@jkrems
Copy link
Contributor

jkrems commented Oct 3, 2017

@ORESoftware It depends on the version of node the running process uses. If it's node 8, then you should be able to kill -USR2 ${pidOfTheNodeProcess} to enable the --inspect behavior after the fact.

@ORESoftware
Copy link
Contributor

@jkrems thanks, how does that actually work?

https://lists.freebsd.org/pipermail/freebsd-questions/2007-August/156889.html

I guess Node.js itself defines that signal as to allow --inspect to work after the fact. I remember learning about this a few years ago.

@jkrems
Copy link
Contributor

jkrems commented Oct 3, 2017

@ORESoftware Yes, exactly. node registers a "signal handler" that is called whenever the operating system (e.g. via a user running kill) sends a specific signal to the process. The signal handler for SIGUSR1 is registered here:

RegisterSignalHandler(SIGUSR1, StartIoThreadWakeup);

It will enable the inspect protocol using the options provided at startup. Which means that you can't influence the port for example at the time you're sending the signal (since there's no additional data for signals).

@mizzao
Copy link

mizzao commented Jan 11, 2018

you should be able to kill -USR2 ${pidOfTheNodeProcess} to enable the --inspect behavior after the fact.

@jkrems I think you mean kill -USR1 <pid>. USR2 causes a heap dump.

@eugeneo
Copy link
Contributor

eugeneo commented Jan 11, 2018

More portable way would be to use process._debugProcess. E.g.:

~ $ node &
[1] 194811
~ $ node -e "process._debugProcess(194811)"

Upd: from the REPL, you may also call it on yourself - process._debugProcess(process.pid)

@jkrems
Copy link
Contributor

jkrems commented Jan 11, 2018

@mizzao Thanks! Yes, I can never remember which is which. Really should've double-checked. :)

Good point re: portability w/ process._debugProcess. We should consider turning that into a non-underscored method so it doesn't look quite as dirty.

@mizzao
Copy link

mizzao commented Jan 11, 2018

What's the difference between

node -e "process.debugProcess(<pid>)"

and

node inspect -p <pid>

?

@eugeneo
Copy link
Contributor

eugeneo commented Jan 11, 2018

First starts a WebSocket server so you may attach Chrome DevTools or other frontend. Later starts a command line debugger (that uses same WS server)

@mizzao
Copy link

mizzao commented Jan 11, 2018

Oh, I see. So it's equivalent to kill -USR1 <pid>?

@eugeneo
Copy link
Contributor

eugeneo commented Jan 11, 2018

That's exactly what it does on *NIXes. It gets more complicated on Windows (kill is n/a on Windows at all)

@june07
Copy link

june07 commented Nov 3, 2018

@jkrems Looking for a little guidance here...

Is there a way to influence (ie change) the inspect port when starting the inspect web socket after runtime? Maybe process._debugProcess(process.pid) would be a good place to start looking/make changes? I've dug around a bit and I'm not sure exactly where to begin in the codebase... C++ or Javascript. My C++ is weak so I'm sure I'm missing a lot in that regard. Some expert help/finger pointing would be great!

My problem currently is coming from the fact that you can only start the inspect process pragmatically on the default port 9229 and that's it. I'd like to be able to start debug web sockets on random ports, such that doing so would be possible on more than a single running Node program.

Another question I had was... is it possible to programmatically shut down the web socket externally just the same but opposite to the way it was started up (ie SIGUSR1)?

@ORESoftware Yes, exactly. node registers a "signal handler" that is called whenever the operating system (e.g. via a user running kill) sends a specific signal to the process. The signal handler for SIGUSR1 is registered here:

node/src/inspector_agent.cc

Line 130 in 2f8ddb2

RegisterSignalHandler(SIGUSR1, StartIoThreadWakeup);
It will enable the inspect protocol using the options provided at startup. Which means that you can't influence the port for example at the time you're sending the signal (since there's no additional data for signals).

@jkrems
Copy link
Contributor

jkrems commented Nov 4, 2018

My problem currently is coming from the fact that you can only start the inspect process pragmatically on the default port 9229 and that's it.

The problem is a basic property of unix signals: They don't pass any data. So from the outside there's only "send USR1" which is all process._debugProcess does in the end. What you can do is start the processes with --inspect-port=<unique port>. That will make sure that once you send the signal, they will start listening on that custom port.

Another question I had was... is it possible to programmatically shut down the web socket externally just the same but opposite to the way it was started up (ie SIGUSR1)?

That might be possible but is tricky for two reasons:

  1. It breaks when multiple clients are involved. E.g. an editor and a profiling tool both trying to attach to the same process. The 2nd client would accidentally stop the debug connection, not only preventing itself from working but also breaking the 1st client's connection.
  2. If the debugger is active, the VM might be paused. Which, depending on how the signal is handled, might prevent the signal handling code from running. This is more a technical challenge and might be solvable.

The first one is what makes it most likely not practical to use the same signal for either. What might be possible though is to call process.stopDebug (forgot the exact function) via Runtime.evaluate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 question Issues that look for answers.
Projects
None yet
Development

No branches or pull requests