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

Adapt "auto attach" feature to js-debug #88599

Closed
weinand opened this issue Jan 14, 2020 · 15 comments · Fixed by #95807
Closed

Adapt "auto attach" feature to js-debug #88599

weinand opened this issue Jan 14, 2020 · 15 comments · Fixed by #95807
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality on-testplan
Milestone

Comments

@weinand
Copy link
Contributor

weinand commented Jan 14, 2020

I would expect that using the existing "auto attach" feature of node-debug applies to js-debug too.

@weinand weinand added feature-request Request for new features or functionality debug Debug viewlet, configurations, breakpoints, adapter issues labels Jan 14, 2020
@vscodebot vscodebot bot added this to the Backlog Candidates milestone Jan 14, 2020
@weinand weinand modified the milestones: Backlog Candidates, On Deck Jan 14, 2020
@microsoft microsoft deleted a comment from vscodebot bot Jan 14, 2020
@weinand weinand removed their assignment Jan 15, 2020
@weinand
Copy link
Contributor Author

weinand commented Feb 11, 2020

@connor4312 how do you want to transition this feature to js-debug?

@weinand weinand modified the milestones: On Deck, February 2020 Feb 11, 2020
@connor4312
Copy link
Member

connor4312 commented Feb 11, 2020

I'm curious whether the debug terminal in js-debug fills the niche that auto attach does with the existing debuggers. For me it's become my go-to way to debug code, though I admit that before starting work on debugging here I didn't know about auto-attach so haven't been a heavy user of it. Extra knowledge is needed for both use cases, to use --inspect in the normal terminal, or using the command to open the debug terminal. Thoughts?

@weinand
Copy link
Contributor Author

weinand commented Feb 12, 2020

Yes, the debug terminal is definitively similar to "auto attach".
So IMO we have two options now:

  • we remove the auto-attach feature from VS Code and explain users in the release notes how to use the debug terminal,
  • we use the auto-attach feature as an entry point to the new debug terminal, e.g. when a user knows this setting or even has the setting already turned on, we use it to enable/open the debug terminal for him. Or we use the "auto attach" setting to guide the user to the debug terminal functionality (and then hide the auto attach indicator in the status bar).

I'd prefer the second option because with this we could easily redirect existing users to the new feature.

One technical aspect of the "auto attach" setting to keep in mind:
A user can enable "auto attach" to be on by default. This means that after a restart VS Code actively polls for node.js programs started in the integrated terminal.
Since we do not want to start the node-debug extensions on startup, we have moved the "auto attach" functionality into a lightweight extension that can be launched on startup without slowing the startup time in a significant way (< 10ms). Only if the auto-attach polling detects a node.js in debug mode, it will activate node-debug.
If we want to provide similar "always-on" experience with the debug terminal and js-debug, we should consider a similar approach (and have some js-debug related functionality in the auto-attach extension).

/cc @connor4312 @roblourens @isidorn

@weinand weinand self-assigned this Feb 12, 2020
@isidorn
Copy link
Contributor

isidorn commented Feb 12, 2020

Great work with the Debug Terminal, unfortunetly I did not give it much thought until today - sorry about that.
I agree with @weinand that the Debug Terminal is similar to "auto attach" and that "auto attach" users should be guided to new debug terminal. With the overall goal of dropping "auto attach" in the future.
One additional thing we could do: if user has "auto attach" turned on any new terminal he opens will in fact be a debug terminal.

The biggest issue with the debug terminal is how discoverable it is. Can the js-debug participate in new terminal creation? When a new terminal is created convert it to a Debugger Terminal?
Does the terminal plan to open the title area actions to potential contribute an action there?
Do you plan to potentially contribute this action to the debug start view?

I dislike a command palette (not discoverable) and status bar: too much in your face.

@connor4312
Copy link
Member

connor4312 commented Feb 13, 2020

Participation in terminal creation is something that Kai and I discussed last month while I was working on the debug terminal improvements. I think adapting the "auto attach" button to debug terminals would be a nice way to go, we could have a migration for the existing on-by-default setting. Something like an ITerminalOptionProvider that would have the opportunity to intercept and modify terminal options similar to what the debug configuration providers do today.

I think participation in terminals in this case would be always on (with some user setting to forcefully disable), to set the environment variables and bootloader for each terminal. However, we wouldn't start the debug session server unless the "auto-attach" is on, and have our bootloader no-op if it can't connect to the server. This lets the auto-attach button instantly toggle the whether we debug processes in all terminals, without users having to create a new terminal or close old terminals if they no longer want to auto attach.

I will also need to do some work to make sure that explicitly using the --inspect flag doesn't interfere with our bootloading process.

Related: #46696 (comment)

@isidorn
Copy link
Contributor

isidorn commented Feb 14, 2020

Ok. Please note that how to enable / disable this behavior might be an interesting topic for the UX meeting. For instance I am not a big fan of having the status bar entry, so if we could get something in the actual terminal that would be great imho.

@weinand
Copy link
Contributor Author

weinand commented Mar 16, 2020

@connor4312 what is the current state of your thinking?

I'm trying to understand what an existing "auto-attach" user would experience with the new js-debug.

Ideally it should "just work": she opens a terminal (which automatically has the bootloader enabled) and runs a node.js based program from the command line. That results in a new debug session.

In addition starting up VS Code with "auto-attach" enabled, does not activate js-debug on startup but only when something is run from the (debug) terminal (the old approach of activating node-debug only when needed no longer works with the new bootloader approach because here an active js-debug is already required, correct?)

@Tyriar
Copy link
Member

Tyriar commented Mar 16, 2020

@weinand extensions would be able to contribute environment variables to terminals with #46696, js-debug would use this to set an env var which forces debugging for all node processes. This has the great benefit of no more polling the process tree.

@weinand
Copy link
Contributor Author

weinand commented Mar 16, 2020

@Tyriar yes, I'm aware of #46696. But we'll have to make sure that js-debug does need to be an "activate on startup" extension.

@Tyriar
Copy link
Member

Tyriar commented Mar 16, 2020

@weinand we're discussing that, I think the current thinking is the extension's env contributions are cached between start up. I have concerns about that but it does work around the activate on startup problem..

@weinand
Copy link
Contributor Author

weinand commented Mar 16, 2020

The alternative is to use the existing autoAttach extension as a companion extension to js-debug too. autoAttach is very lightweight and activates on startup. So it could easily contribute the bootstrap env variable (even dynamically by code).

@connor4312
Copy link
Member

connor4312 commented Apr 21, 2020

I'm working on this now. I have it set so that js-debug sets the environment variables that injects the bootloader to debug processes. Thanks to the work Daniel did, this can be persistent between sessions so that we don't need to activate and inject the variable again each time.

However, we do need the extension to activate when the first Node.js script is run in a terminal, so that we can detect and debug it. My thought is to tap into the existing auto-launch extension and use the js-debug pathway if the user has debug.javascript.usePreview or the Node useV3 flag on (or should we use a separate setting?). If this setting is on, auto-attach will listen on a predefined named pipe/socket. The bootloader script will write some metadata to the socket when it's ready to be debugged. This will cause auto-launch to run a command to activate js-debug and trigger debugging. (There's some extra steps we take once we get a session that I would rather keep consolidated inside js-debug, which is why I don't want to startDebugging from directly auto-launch.)

Setting environment variables needs to be done in js-debug so that they're automatically updated if the extension is uninstalled or updated.

In summary:

Turning on auto attach:

  1. extension.js-debug.setAutoAttachVariables sets variables for the workspace, and returns the socket to listen on.
  2. auto-launch listens on the returned socket and stores it in its workspaceState

Turning off auto attach:

  1. Run extension.js-debug.clearAutoAttachVariables to unset variables and turn off any running server.

Receiving a debug target

  1. Auto-launch listens to on the socket when auto attach gets turned on, or it previously had a socket in its workspaceState.
  2. Bootloader connects and writes watchdog info as JSON to the socket
  3. Auto-launch reads that and runs extension.js-debug.autoAttachToProcess

@weinand does this approach + adding it to the existing auto-launch make sense to you?

@weinand
Copy link
Contributor Author

weinand commented Apr 21, 2020

@connor4312 overall the approach sounds good to me.

What I could not clearly see in your summary is what happens for users that currently have auto-attach on. Are they transparently "migrated" to the new Node.js debug terminal?

@connor4312
Copy link
Member

connor4312 commented Apr 21, 2020

What I could not clearly see in your summary is what happens for users that currently have auto-attach on. Are they transparently "migrated" to the new Node.js debug terminal?

They would be transparently migrated to the 'new' terminal/strategy once they turn the debug.node.useV3 or debug.javascript.usePreview flag. We could alternately or also have a separate flag for this, I don't feel strongly: with those flags on, type: node attachments will already redirect to V3 by default, a flag would effectively only toggle the attachment strategy.

@weinand
Copy link
Contributor Author

weinand commented Apr 21, 2020

Sounds perfect.

connor4312 added a commit that referenced this issue Apr 21, 2020
This modifies the debug-auto-launch extension to trigger js-debug as
outlined in #88599 (comment)

Since we now have four states, I moved the previous combinational logic
to a `transitions` map, which is more clear and reliable. The state
changes are also now a queue (in the form of a promise chain) which
should avoid race conditions.

There's some subtlety around how we cached the "ipcAddress" and know
that environment variables are set. The core desire is being able to
send a command to js-debug to set the environment variables only if they
haven't previously been set--otherwise, reused
the cached ones and the address.

This process (in `getIpcAddress`) would be vastly simpler if extensions
could read the environment variables that others provide, though there
may be security considerations since secrets are sometimes stashed
(though I could technically implement this today by manually creating
and terminal and running the appropriate `echo $FOO` command).

This seems to work fairly well in my testing. Fixes #88599.
connor4312 added a commit that referenced this issue Apr 23, 2020
* debug: enable js-debug to auto attach

This modifies the debug-auto-launch extension to trigger js-debug as
outlined in #88599 (comment)

Since we now have four states, I moved the previous combinational logic
to a `transitions` map, which is more clear and reliable. The state
changes are also now a queue (in the form of a promise chain) which
should avoid race conditions.

There's some subtlety around how we cached the "ipcAddress" and know
that environment variables are set. The core desire is being able to
send a command to js-debug to set the environment variables only if they
haven't previously been set--otherwise, reused
the cached ones and the address.

This process (in `getIpcAddress`) would be vastly simpler if extensions
could read the environment variables that others provide, though there
may be security considerations since secrets are sometimes stashed
(though I could technically implement this today by manually creating
and terminal and running the appropriate `echo $FOO` command).

This seems to work fairly well in my testing. Fixes #88599.

* fix typo

* clear js-debug environment variables when disabling auto attach
@github-actions github-actions bot locked and limited conversation to collaborators Jun 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality on-testplan
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants