Node: regression: 3rd party debuggers are incompatible with node8 nighlies

[roblourens]

In https://github.com/nodejs/node/pull/12197, support for --inspect --debug-brk was removed, and --inspect-brk should now be used instead, but --inspect-brk is only supported after 7.6.0.
Since there is no common way to start the inspector across all Node versions that support it, this is an issue for VS Code and other debug clients which now have to determine which version of Node they are launching and select the right arguments, or detect when using one set fails, and try the other set. It's also annoying for anyone who switches node versions often and uses these arguments from the command line.

Would it be possible to retain support for --inspect --debug-brk so that there's one command which can start Node in debug mode across all versions that support the inspector protocol? If it simplifies things, it could be undocumented.

[roblourens]

@joshgav Any thoughts on this? I thought I read at one point that --debug would be an alias for --inspect in the future but I guess that's not the case anymore.

[sam-github]

Keeping uniform behaviour across all current LTS lines is quite helpful. Once all LTS nodes support --inspect-brk, users can switch over to it.

[evanlucas]

yea, I'm +1 on this

[gibfahn]

So the logic is that it's quicker to re-add --debug-brk to Node 7 than to backport --inspect-brk to Node 6 and 4 (and 4 is in maintenance so may never get it)? Makes sense to me.

[sam-github]

We could backport --inspect-brk to node 6, that would allow --debug-brk to be removed from current as soon as 4.x goes out of maintenance, rather than having to wait until 6.x goes out of maintenance.

[sam-github]

@MylesBorins @nodejs/lts what do you think of above? Should we discuss in WG meeting, or should I PR a backport?

[addaleax]

@sam-github If you want to, I think opening a PR is a good idea. There isn’t really any need to wait for a meeting if there is consensus that it should happen.

[ofrobots]

So the logic is that it's quicker to re-add --debug-brk to Node 7 than to backport --inspect-brk to Node 6 and 4 (and 4 is in maintenance so may never get it)? Makes sense to me.

--inspect is only supported since 6.3, so I don't think 4 is relevant here.

From my reading, I think the argument is slightly different than the way you put it. Without knowing the precise version upfront, I think it is hard to know whether --inspect-brk would work. It will never work for users of 6.4.0 for example because a backport would only get into 6.11.0 at the earliest.

@roblourens wouldn't you still need to distinguish Node <6.3.0 from >=6.3.0?

[sam-github]

@ofrobots, good point. I didn't realize the inspector only got introduced halfway into 6.x. So, we'll need to get the backwards-compatible --inspect --debug-brk back, and keep it until 6.x is EOL.

[gibfahn]

@ofrobots, good point. I didn't realize the inspector only got introduced halfway into 6.x. So, we'll need to get the backwards-compatible --inspect --debug-brk back, and keep it until 6.x is EOL.

@sam-github --inspect --debug-brk works fine in v6.x, --debug-brk was removed in master as a semver-major, so the question is whether we re-add it to master and Node 8.

Either way we should backport --inspect-brk to v6.x.

[sam-github]

--inspect --debug-brk works fine in v6.x

In later half of 6.x, yes, but it wasn't there initially.

And yes, I agree:

• --debug-brk should be added back to master and node 8.x
• its worth backporting --inspect-brk to 6.x

[roblourens]

@ofrobots That's right, thanks. We do still need to do version detection for which debug protocol to use, but only in the simple case when we run with Node on the user's path. But if the user provides another "runtimeExecutable", which might be a path to a shell script or 'npm' or another version of node, it's not safe to invoke that with --version, and that won't work 100% of the time anyway. So then we rely on the user to set "protocol": "inspector" to debug with --inspect. Changing the argument names would introduce another complicating wrinkle.

[refack]

Wait so in v6 --debug-brk uses debug protocol
in v7 --debug-brk uses the debug protocol
but --inspect --debug-brk uses the inspector protocol?

and node4:

C:\code\node> node4 --inspect --debug-brk
node4: bad option: --inspect

@roblourens how do you deal with this? Don't you feel fixing this issue will be a bandaid?

[refack]

So the logic is that it's quicker to re-add --debug-brk to Node 7 than to backport --inspect-brk to Node 6 and 4 (and 4 is in maintenance so may never get it)? Makes sense to me.

@gibfahn It's a wrong assumption.

1. All existing version of v4 will only work with debug protocol, and don't support --inspect --debug-brk
2. Some existing versions of v6 don't support the --inspect --debug-brk and some do.
3. All version of v7 support --inspect --debug-brk but some also support --inspect-brk
4. And the current decision is that node 8 will only support --inspect-brk

So restoring the --inspect --debug-brk will not solve all of @roblourens's problem. I'd rather go help his project then revert back to the --inspect --debug-brk combo forever.

[gibfahn]

@refack node --inspect --debug-brk will work across all versions of Node that have the inspector (if we re-add to Node 7 and Node 8), except for a couple of Node 7 versions. That doesn't solve all the problems, but it makes things simpler right? And the problem isn't just for Jetbrains, it's for Node devs who want less mental load. What's the downside to keeping this alias?

And the current decision is that node 8 will only support --inspect-brk

That decision hasn't been made, it'll probably happen in https://github.com/nodejs/node/pull/12580.

[refack]

I made contact with JetBrains they have the same issue, but FWIW they already do version detection and select which parameter to pass. They claim version detection is robust, and had received no user complaints about the edge-case of a wrapper script as "runtimeExecutable".
I also opened Microsoft/vscode-node-debug2#100 to help @roblourens with a solution on his end.

@gibfahn I believe this was discussed ad nauseum... nodejs/node-inspect#43, #12197, nodejs/CTC#94, nodejs/CTC#40, nodejs/diagnostics#67, #7266, #10276, #10187, #11770, #11207, #11431, #12352, #11421, #11441

I understand it was a "fast" deprecation because of V8 constraints, but still after considerable thought decisions were made, and I don't think this issue if enough to revert those decisions, and for a non optimal ad-hoc solution.

[MylesBorins]

Wouldn't the simplest solution be to simply keep --debug-brk as an alias for --inspect-brk in 8 and just leave it at that?

[refack]

Comment are welcome (I'll incorporate them into this summary)

/cc @nodejs/ctc
I would like the CTC to review the proposed solutions that have came up so far:

1. ##### Have --inspect --debug-brk as a uniform way to trigger debug on first line
   
   
   
   1. Restore the --inspect --debug-brk combo to v7+v8 (#12580)
   
   
   2. Does it stay undocumented? Or maybe it should become the primary option and remove --inspect-brk (re: #11441)
      
      (IMHO if we keep it --debug-brk, --inspect-brk will probably never be used)
   
   
   3. Will the __combo__ be valid in node8?
   
   
   4. What do we do with --debug-port / inspect-port?
   
   
   5. In light of these, consider what to mark as deprecated.
   
   
   6. Since v6 and v7 requires the combo --inspect[=port] --debug-brk[=port] is specifying port on both args a valid invocation, and in that case which port wins?
   
   
2. ##### Have --inspect-brk as a uniform way to trigger debug on first line
   
   
   
   1. Keep the --inspect --debug-brk combo in v7 alone (deprecation notice yes/no) (i.e. don't land #12197 in v7)
   
   
   2. port the --inspect-brk alias to v6 (#12615)
      
      [new comment] this will make --inspect-break a feature of recent versions of 6.x, but it can't change the past: versions of 6.x will always exist without this feature, and so will not be debuggable by third-party tooling [without special treatment]
   
   
   3. for v4 it's irrelevant since it's a different protocol and other means of detection and handling is necessary
   
   
3. ##### Help the users adapt to our plan:
   
   
   
   1. Help fix VSCode properly, make it compatible with our current and future plans (/cc @roblourens Ref: Microsoft/vscode-node-debug2#100)
   
   
   2. Make sure JetBrains handle node8 nightlies (/cc @ulitink @segrey youtrack#WEB-26568)
   
   

User feedback

I'm trying to get more feedback from @roblourens and JetBrains, so you could make the best decision.

1. Quote from youtrack#WEB-26568
   

P.S. at present WebStorm and IDEA based IDEs can't trigger debug in node8 nightlies (nor can VSCode)

[roblourens]

I think my main issue is that if we lose --inspect --debug-brk, lots of people with the currently latest versions of WebStorm and VS Code won't be able to debug Node 8. I'm not sure what WebStorm's release schedule is like, but not everyone upgrades VS Code immediately and it will be unfortunate for something so fundamental to fail. I've been following the discussion since the inspector protocol was introduced and I was still caught off guard by this change. We knew the old debug protocol is going away but didn't realize there would be a breaking change to the inspector protocol CLI args, and apparently I'm not the only one.

Besides that, I can expand on VS Code-specific issues in Microsoft/vscode-node-debug2#100.

[refack]

@roblourens Thank you for your feedback!

[refack]

I want to add an observation:
Current version of the IDEs work with current versions of node.
The case we are talking about is someone who updates to semver-major version of node but does not updates their IDE.
IMHO it's not a significant request from the user to update their IDE when updating a semver-major version of their platform.

@roblourens see you at Microsoft/vscode-node-debug2#100 😉

[sam-github]

Adding back in the backwards compatible --inspect --debug-brk is basically a matter of a single strcmp() in the source, a couple dozen chars, and gives compatibility across everything. If it was maintenance of a rats nest of compatibility code, I'd say its not worth it, but in this case, why should we tell people what versions of their tools to use? Why should people have to change their tools depending on the version of node they use? Why should we have node major branches, where within the same branch, thirdparty tools have to use different CLI flags depending on the exact node version?

Seems a lot of trouble to push onto the ecosystem avoidable with a trivial change on node's part.

I'm in favour of node evolving, but I think we should try to keep it as easy as possible for people to write tooling (or code) that can work uniformly across all current and LTS versions.

Since v6 and v7 requires the combo --inspect[=port] --debug-brk[=port] is specifying port on both args a valid invocation, and in that case which port wins?

I don't see how this is related, it makes it sound like supporting --debug-brk raises some new command line parsing issues, but it does not.

Support for combingin args is pre-existing, and well defined ATM: --inspect-brk=localhost:9990 --inspect-port=10.0.0.1:7654 --inspect=5656 <--- last host:port wins, and the brk behaviour wins.

[refack]

@sam-github I mostly agree, and I assume if debug/inspector protocols change had more time we could have handled this whole issue __way__ more smoothly.
Unfortunately the status-quo is that there are multiple node binaries of multiple versions, and use not only different CLI args, but different communication protocols.
IMHO the main use-case of this issue are the IDEs, and they are communicative, and more "manageable" then the whole eco system.

P.S. After hearing from the IDEs IMHO we should forget about --inspect-brk and keep --debug-brk permanently. Protocol detection & resolution is done in orthogonal ways anyway.

[refack]

@roblourens seem like the wind is blowing in your direction.
We spun off a "meta" issue #12630

[jsumners]

IMHO it's not a significant request from the user to update their IDE when updating a semver-major version of their platform.

I disagree. In particular when talking about Webstorm. Someone may have bought a license for a specific version when they could afford it, but is unable to afford the cost of updates. If the only thing that is changing to break such IDEs is the removal of a switch, I think it is more reasonable to keep the switch people have come to rely upon.

[jasnell]

The switch is not the critical part in that scenario, however. The underlying protocol changed completely. The older IDE won't be capable of supporting it without updates anyway.

[refack]

I disagree. In particular when talking about Webstorm. Someone may have bought a license for a specific version when they could afford it, but is unable to afford the cost of updates. If the only thing that is changing to break such IDEs is the removal of a switch, I think it is more reasonable to keep the switch people have come to rely upon.

@jsumners I'm talking about an update no upgrade. JetBrains and VSCode manage the communication with the nodejs debugger as an updateable plugin.
Our main concern is making sure they have the plugins ready for the release of node8. All the users will need to do is to "accept" the update...

[jsumners]

👍

[roblourens]

@refack Our node debug extension is packaged with the app and can't be updated out of band, so the only way to get a working version will be to update VS Code.

[roblourens]

By the way, what is --debug-port/--inspect-port from https://github.com/nodejs/node/issues/12364#issuecomment-296508427? I don't see where those are documented.

[prigara]

@refack

I'm talking about an update no upgrade. JetBrains and VSCode manage the communication with the nodejs debugger as an updateable plugin.

Node.js plugin is bundled with WebStorm and some other JetBrains IDEs and can not be updated without the IDE update. Normally we do not backport fixes to the previous major IDE versions. Support for --inspect --debug-brk would guarantee that the majority of our users have working Node.js debugger in WebStorm.

[refack]

@roblourens @prigara I stand corrected.
But I'll reiterate the statement that we want to make sure your products are ready for node 8. Please see dedicated discussion in #12630 (I'll quote your two last statements)

P.S. @roblourens --debug-port is an undocumented switch that sets the debug port explicitly, it was meant for IDEs and external debuggers, but I guess we failed to communicate that. (see node_debug_options.cc#L104)

[refack]

@roblourens I want to change the title of this issue to:
__regression: 3rd party debuggers are incompatible with node8 nighlies__
are you ok with that?

[sam-github]

@refack you as a collab can fixup issue titles, the fact that you did shows up in the conversation thread so the new text (spelling errors, etc. :-) won't be misattributed to the opener of the issue, and often issue titles stand a bit of touching up once the problem is better understood. That is, go for it.

[sam-github]

I understand your hesitation. @jasnell reworking bug report titles for clarity seems to be part of the community care we do to keep the issue tracker in good shape, not so different from adding appropriate labels, would you agree?

[jasnell]

yep, updating titles happens all the time.

[refack]

Workaround for the meanwhile:
https://gist.github.com/refack/ab07a28580672a67f8e1e0adde359aba