Reintroduce js-debug auto attach

[connor4312]

We swapped auto attach to js-debug in the June release with destructive results, and so reverted back to node-debug's auto attach in a recovery.

• Referenced elsewhere, we made auto attach work all the time for all process. This is neat but was highly disruptive to users who were previously used to auto attach only working when they passed --inspect flags manually.
• We also introduced, prior to making the call to disable it, implicit vs explicit auto attach, where in explicit mode we would only attach if --inspect is given.

We'd like to re-enable js-debug auto attach this coming iteration:

• Auto attach mode
  • @kieferrm brought up that auto attaching this way is a super nice way to discover debugging without knowing anything else.
  • One approach we could take is migration to a new auto attach setting, where users that had auto attach on previously default to "explicit" attach mode (only attaching if --inspect is already given) and new users get implicit auto attach.
  • Personally I would prefer to use explicit auto attach, as many Node commands I run in the console, like npm install / builds, I don't want to debug.
  • A third option would be to add Python-esque "run" editor actions. This is not related technically to auto attach, but also serves the purpose in having a clear 'default' way for users to run code with debugging enabled.
• ENOENT issues
  • The introduction of the bootloader previously caused a cascade of issues where the bootloader went missing. Most of the time this was because we observed a Node 10 install and, using logic shared between debugging and auto attach, therefore hoisted the bootloader into a tmpdir.
  • We should instead only enable auto attach if either there are no spaces in the bootloader's path (so hoisting is not needed) or we verify the user is running Node 12 or higher.
  • By default we tried to keep the bootloader in workspaceStorage. However, it seemed that in remote cases this could go missing even when environment variables for the workspace were preserved. This needs to be investigated before we can proceed.
    • Is keeping the file in the extension dir better? But this will break nightly builds where we have a new version and folder each day.
    • Alternately, could the built-in debug auto launch extension copy/store the bootloader for us in a more permanent location?

[connor4312]

Another option brought up in standup: attach based on the script being run. E.g. don't attach if running "npm" or "webpack", but do attach if running "node" directly.

[connor4312]

In the next nightly (which will work on the following Insiders, opt-in), I have done the following:

• Added a nice, verbose error if there's a space in the workspace storage path and we aren't verifiably running Node 12.
• Added a default-on "smart" mode that only attaches either when running a script that's not a locally or globally installed dependency, or when --inspect is manually given to the process. Note that this still works when calling through global scripts, e.g. npm start will work and just skip debugging the npm wrapper.

Following up on ENOENT, looking back at the issues. The vast majority were either:

• Versioning/version detection issues
• Tmpdir issues

There was only one case where the workspace storage might have gone missing, and that could have been a version detection issue. Thus I am not sure if that is a problem. Looking at the terminal variables code in VS Code, it seems like they're kept in the workspace storage as well.

Once Insiders is published tomorrow I will go through a bunch of test VMs and setups to make sure everything is solid.

[connor4312]

Further item from standup today: allowlist common test runners to enable auto attach for them. Also, ts-node.

[connor4312]

Also, cc @weinand in case you're interested in following this 🙂

[weinand]

@connor thanks for cc-ing me.

Yes, I vote for "explicit auto attach" because that's the way it worked before, and existing users understand how that works. In addition it is less aggressive in debugging everything (like for example the .bashrc scripts that happen to use node.js scripts ;-) .

I wasn't aware of the bootloader's fragility with respect to spaces in paths and node.js versions. Showing a notification in these situations is definitively better than disabling auto-attach silently.
My approach for node.js version problems was always to not bother the user with that because there are still users that have to use legacy versions of node.js for a reason and we should support them.

"attach based on the script being run" sounds "too magic" for my taste and is not really transparent. Who owns the list of magic commands for which auto-attach is suppressed? Are users able to add new commands to that list?

I suggest to keep the bootloader in a location that does not have a path that contains the user name.
For named pipes we use the tmp directory - why not using that for the bootloader too?
At least on macOS the tmp directory path is guaranteed to not contain spaces.
Using the built-in debug auto launch extension to copy/store the bootloader is a good idea. Go for it..

[connor4312]

Yes, I vote for "explicit auto attach" because that's the way it worked before, and existing users understand how that works. In addition it is less aggressive in debugging everything (like for example the .bashrc scripts that happen to use node.js scripts ;-) .

"attach based on the script being run" sounds "too magic" for my taste and is not really transparent.

I was originally in favor of that as well, but Kai's convincing point was that only a small percentage of VS Code users use debugging at all. While defaulting to "explicit" attach mode as we do today makes these users happy, it does not help the vast majority of users who don't know about/use debugging. But if I run node my-first-program.js in VS Code and get debugging automatically, that is a great way to onboard without needing any external knowledge, about launch configs or even the debug terminal.

Meanwhile, auto attaching to build scripts and such is very annoying, so attaching to everything is not a good solution. Thus I landed on the smart attach mode.

Who owns the list of magic commands for which auto-attach is suppressed? Are users able to add new commands to that list?

Currently smart attach will only attach when running scripts not installed globally and not in node_modules -- so it's suppress by default. For example, this prevents attaching to tools like webpack, tsc, or npm.

We would have an allowlist of modules where we want to lift this surpression, such as mocha, jest, or ts-node. I expect to receive a steady trickle of PRs adding more tools to the list over time. Making it configurable in a setting is a good idea -- we could add a denylist as part of that as well.

I wasn't aware of the bootloader's fragility with respect to spaces in paths and node.js versions. Showing a notification in these situations is definitively better than disabling auto-attach silently.
My approach for node.js version problems was always to not bother the user with that because there are still users that have to use legacy versions of node.js for a reason and we should support them.

I suggest to keep the bootloader in a location that does not have a path that contains the user name. For named pipes we use the tmp directory - why not using that for the bootloader too?

That's what we did originally. The problematic thing is that the OS could clear the temp directory while the environment variables were still present in the VS Code terminal. This would cause all Node commands run in the integrated terminal to fail when they could not find the module (e.g. microsoft/vscode#102557)

Because this breaks in such a severe way, in re-enabling js-debug auto attach I wanted to be very conservative with the way we enable it.