Prepare for node -> node2 transition

[roblourens]

Since the legacy node debug protocol is on its way out, we need to continue preparing a seamless transition to the inspector debug protocol.

Status:

• node --debug will print a deprecation message, probably in some 7.x release - lib: deprecate node --debug at runtime nodejs/node#10970
• In Node 8, sending a SIGUSR1 signal to a node process will switch it into debug mode using the inspector debug protocol - Debug already running process using v8-inspector? nodejs/node#8464
• The legacy protocol is completely removed in V8 5.8 - Legacy JSON debug API removed as of V8 5.8 nodejs/node#9789
• It's not clear whether Node 8, released in probably April, will have V8 5.7 or 5.8. Seems likely that it will be 5.7 but we should be ready regardless - Node.js 8.0.0 Tracking Issue nodejs/node#10117

Possible February plan:

• Option 1:
  • Wrap node and node2 in a single debug adapter. It checks the node version to be launched or attached to, and delegates to the proper debug adapter, using node2 when possible.
  • It also registers 'node_legacy' to force the legacy debug adapter
• Option 2:
  • Change 'node' to detect the node version and warn the user to change the type to node2 if it's after 8
  • Then at some point in the future, rename node2 to node

It would be good to have a seamless experience like in option 1 - a user doesn't care which debug protocol they use. That should be our job, if possible. There are some issues to deal with though.

• When runtimeExecutable is specified, we will have to leave it up to the user. We can't always figure out its node version. It might be a script or something that isn't node. It might be electron, which ships a version of node 6.5 that doesn't include --inspect. We can probably figure out what this failure looks like, and give a helpful warning.
• When attaching, we should be able to figure out which protocol it's using. One easy difference is that under the inspector protocol, we can make an http request to http://localhost:port.

Other things to do:

• Remove the 'experimental' tag on node2
• Implement attaching to a running process using node2 - Can't put running Node process into debug mode vscode-node-debug2#15
• Localize node2 (strings already go through a dummy localize function, just need to actually hook it up to nls and get translations) - Localize node2 vscode-node-debug2#83

FYI @weinand @joshgav

[weinand]

Wrapping both adapters into one "smart adapter" has the problem that all feature negotiation between the frontend and the debug adapter (DA) happens before the adapter knows what Node.js version is being used. It's difficult for the combined DA to return the correct capabilities if it doesn't know whether it will turn into 'node' or 'node2' when it eventually attaches to Node.js.

For this reason I propose that we fold both adapters into a single extension in a "semi-smart way":

• we only have a single debug type node (and consequently only a single type of launch configuration attributes). So the user never has the choice between two different debug types or debug environments. This applies to snippets and variables as well: there is only a single version of them.

• the node-debug extension contains the two DAs and we use the new 1.9.0 adapterExecutableCommand feature to determine dynamically what adapter gets launched. The strategy to determine which adapter to use works like this:

  • if the launch config specifies a new attribute (name TBD but may be protocol and values CDP and legacy) we use the corresponding debug adapter.
  • if nothing is specified we determine the version of the currently installed Node.js and use the CDP-DA for version >= 6.5 (or whatever).

  This approach is 'semi-smart' because we are determining the DA early and not when doing the 'attach'. That means our approach might fail in situations where we debug Node.js remotely and the local node version does not match the remote version. On the other hand this approach does not require a forwarding DA with delayed handling of feature negotiation and is most likely more robust.

Since this transitions is non-trivial and involves many details, I suggest that we do not rush this through in a single milestone but plan for a staged, two milestone approach.

I propose the following steps:

• Since the approach is based on the assumption that we can use launch configurations for both DAs interchangeably (because there will be only a single JSON schema), we should verify this and fix issues as far as possible. If some attribute differences remain (and are not resolvable), we will make this information available in the hover-help information for the attribute.

• we enhance both adapters to have good error reporting for the case that the DA is used against a wrong node version (because the user will have to change his launch configuration in this case).

• we add the node2-DA to the node-debug extension either by:

  • moving the source from the vscode-node-cdp-debug repo into the vscode-node-debug repo
  • keeping both repos but enhancing the package build step in vscode-node-debug to copy the node2-DA resources into the vsix package (my preferred solution).

• we write the code in the extension.ts that determines what adapter to use. If we have confidence that users will not experience issues if they are automatically upgraded to the node2-DA, we can enable this code. Otherwise we can continue to use 'node' by default for another milestone and let the user decide when to upgrade via the protocol attribute manually.

/cc @isidorn @egamma @kieferrm

[weinand]

It just occurred to me, that we could use the approach from above even if we keep both debug extensions, but just remove (comment out) the "debuggers" contribution from 'node-debug2'.
With this 'node2' will not appear anywhere in the UI but the code that determines what adapter to launch can still access the adapter in the node2 extension.
This would be a cheap way to verify that the overall approach works.

[roblourens]

That's a good idea. I don't think there will be a problem with the launch configs being compatible. You made me think of another problem with having one debugger type for the two adapters, which is that static differences like the callstack context menu contribution will appear for both, but do nothing for 'node', unless we add a way to hide it.

I think adapterExecutableCommand doesn't get the launch config right now, so we can change that and use it like you said. But I don't think attach mode is a problem because we can detect whether the remote end is using the new protocol by making an http request to the address and port.

Maybe an intermediate step for February could be to deprecate "type": "node2" telling people to use protocol, and implement protocol with the values legacy, v8-inspector, or auto, and we only do the autodetect logic if people opt-in to it. Then we have plenty of time to shake out any issues before springing this on everyone's 'node' launch configs.

[roblourens]

Or we could do the autodetect logic in startSessionCommand instead of adaterExecutableCommand, and it could set the type to 'node' or 'node2', which would then invoke the proper debug adapter.

[joshgav]

@roblourens

One easy difference is that under the inspector protocol, we can make an http request to http://localhost:port.

You might also query http://localhost:9229/json/version, which returns a JSON object with Browser and Protocol-Version properties (see https://github.com/nodejs/node/blob/81d15594d7cd08be1990ad6341a3491a34e20d63/src/inspector_socket_server.cc#L114-L119).

Output on my system:

{
  "Browser": "node.js/v7.4.0",
  "Protocol-Version": "1.1"
}

[weinand]

@roblourens I've added support for a 'protocol' attribute. If 'protocol == auto' I'm probing for the node version by running node --version and use node2 for versions >= 6.5. Feel free to add more/better probing.

To simplify alignment of required attributes between the two adapters, I've made the cwd attribute optional and I'm supplying a value in the startSessionCommand. I've removed the cwd from all snippets and initial configurations.

Since the 'node2' extension contributes snippets and initial configurations there is a chance that node2 appears in the UI. I suggest that we remove these contributions as soon as we have verified that we are not loosing functionality (because we are covered by the contributions in node-debug).

For the callstack context menu contributions I'll add a corresponding implementation for node-debug.

[weinand]

@roblourens probably the most valuable addition to the version probing would be for the case 'request' === 'attach': in this case you know the port and can do the approach @joshgav mentioned above.

This would have solved the problem I was running into when using the "Attach to Extension Host" launch config of VS Code. It was using 'node2' which didn't worked because the EH uses the Electron node which does not support the '--inspect' option.
(I've fixed the launch configs for now by adding a 'protocol': 'legacy')

[roblourens]

Agree, I'll work on that

[roblourens]

Done. I changed the minimum version for node2 to 6.9 because it crashed a lot on windows before that, and there was some flakiness on other platforms too.

I didn't do anything different with the 'runtimeExecutable' case because checking the version of node on the path is probably the best thing to do most of the time. Maybe we should log a message when the node version is inferred though. I'll open an issue for it.

[weinand]

@roblourens great, thanks!

I noticed that you do the checkAttach() even if the user has set config.protocol to legacy or v8-inspector. Is the plan that you want to report an error if the user's choice is in conflict with the result of your probing? If not then I suggest to do the checkAttach() only in the auto case.

[roblourens]

I actually only do it if !config.protocol. But I need to also do it when protocol is auto...

[weinand]

Indeed, so your checkAttach() is just the alternative for attach in the default/auto case of the switch.

[weinand]

The attach probing is tricky...

If I start node v6.5 with this:

node --inspect --debug-brk server.js 

attach will not work because the probing code sees a version < 6.9 and decides that 'v8-inspector' is not supported. But the 'legacy' DA doesn't work either, because debugging was not enabled for the legacy protocol (due to the '--inspect' flag). So there is no fallback at all.

I think we'll have to use the 'v8-inspector' whenever the 9229 port returns a valid json (independent from the version).
But if nothing is returned from that port, we can assume that either node does not support the new protocol at all, or it was not started with the '--inspect' flag. But attaching with 'legacy' and the same port (9229) would not work either because node is most likely listening on port 5858 instead. But changing the port number automagically doesn't sound right.

Maybe we should use a port '5858' as an indication for 'legacy' and '9229' for 'v8-inspector'. And only for all other values we would do the probing.

[roblourens]

Oh yeah, that's a good point - for attach we need to support any node version with the v8-inspector protocol. That's not a problem.

I've seen people use 5858 for debugging with node2, so I don't want to make any assumptions based on the port number. It seems seems fine as-is to check whatever port they specify for a v8-inspector server.

[roblourens]

I just pushed a change to rename "v8-inspector" to "inspector", to ensure it's a runtime-independent name.

[weinand]

I've created these two items: microsoft/vscode-node-debug2#87 and microsoft/vscode-node-debug2#88

[roblourens]

Also filed microsoft/vscode-node-debug#130 on myself to continue investigating.

[weinand]

Everything planned for this milestone has been done.