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

Auto attach debugger to Node processes started from integrated terminal #42521

Closed
auchenberg opened this issue Jan 30, 2018 · 25 comments
Closed
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality on-testplan
Milestone

Comments

@auchenberg
Copy link
Contributor

auchenberg commented Jan 30, 2018

Scenario
I as a Node developer use the integrated terminal to launch my Node.js app as I'm used to. My app starts, and now I want to debug. I could make a debug configuration to launch my Node app via VS Code, but that would be a new workflow for me. Instead I want VS code to be able to attach to my started Node process from the terminal.

Proposal
We should explore auto attaching to our Node debugger to the first Node process started from the integrated terminal.

Flow:

  1. User starts process from integrated terminal. Ex npm start
  2. User press F5
  3. We auto detect the Node process started from the integrated terminal
  4. Attach to that specific pID via our existing attach/pickProcess configuration.

This could either happen automatically as a part of our magic out of box experience, or through a new property in the debug configuration.

        {
            "type": "node",
            "request": "attach",
            "name": "Launch Program",
            "attachToIntegratedTerminal: true
        }

Rationale

  • Encourage more users to use the integrated terminal.
  • Leverage the benefits that we know which Node processes that were started from VS Code.
  • Provide a end-to-end scenario without forcing developers to change their workflow by having them adapting launching from VS Code.
@weinand weinand added this to the Backlog milestone Jan 30, 2018
@weinand weinand added debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality labels Feb 2, 2018
@auchenberg
Copy link
Contributor Author

Suggestion from @roblourens: "What if in the existing process picker, we find processes that were started from the integrated terminal, and sort them to the top/highlight them?"

@weinand weinand modified the milestones: Backlog, February 2018 Feb 5, 2018
@weinand
Copy link
Contributor

weinand commented Feb 12, 2018

@auchenberg I've started a "Playground" extension for experimenting with this feature.

Since there is no API for accessing existing terminals, the extension just tries to automatically attach to all node runtimes that are in debug mode.

Ideas for more features:

  • auto attach to node runtimes that are not in debug mode (via SIGUSR1),
  • add a QuickPick for selecting potential candidates.

@Tyriar
Copy link
Member

Tyriar commented Feb 12, 2018

Here's the feature request to expose all the terminals to the API #13267, it's mainly just been waiting for compelling use cases. I've wanted to track the terminal process trees for a while as well to give a better experience for confirmOnExit, Windows already does this to track the active process title in the dropdown.

@auchenberg
Copy link
Contributor Author

auchenberg commented Feb 12, 2018

I like that experience @weinand, as the developer intentionally have chosen to enable ``--inspect`.

We could have the out of box experience like:

  1. Auto detect and attach to Node processes started with debugging (like your extension)
  2. Bump Node Child processes to the top in the QuickPick list when using an attach pickProcess config

I still can't decide whether pressing F5 for Node apps where we know a Node child process has been launched from the terminal should change behavior from today, as you can argue both ways.

That said I can easily imagine a workflow where I from my terminal type npm start, followed by a F5 where I then see the QuickPick list. I select my node process, and we send a SIGUSR1 to it, and attach the debugger. That way we don't have an intrusive experience, but we are more intelligent when you press F5 to enable debugging.

Today the experience somewhat falls apart if you already have started from the terminal, as we would try to launch the Node process again, and it would most likely fail as the port already is used.

This would be a change in the default F5 logic with no launch.json:

  • If Node child process detected -> Attach with pickProcess config
  • If no child process detected -> Attempt to launch like today.

What do you think?

@weinand
Copy link
Contributor

weinand commented Feb 26, 2018

@Tyriar tracking process trees for VS Code or the integrated terminal works as long as child process are created under a well known VS Code or integrated terminal process. But when running "tmux" in the integrated terminal, spawned process becomes children of a remote tmux server. So they are no longer part of the VS Code tree. Do you have any ideas how to track those processes?

@weinand
Copy link
Contributor

weinand commented Feb 26, 2018

The February exploration has been finished. For the next milestone it is planned:

  • to add some of the logic to the node-debug process Picker
  • to make the prototype a special mode of node-debug that can be turned on or off.

@weinand weinand modified the milestones: February 2018, On Deck Feb 26, 2018
@weinand weinand modified the milestones: On Deck, March 2018 Mar 6, 2018
@weinand weinand changed the title Explore auto attach debugger to Node processes started from integrated terminal Auto attach debugger to Node processes started from integrated terminal Mar 6, 2018
@Tyriar
Copy link
Member

Tyriar commented Mar 13, 2018

@Tyriar tracking process trees for VS Code or the integrated terminal works as long as child process are created under a well known VS Code or integrated terminal process. But when running "tmux" in the integrated terminal, spawned process becomes children of a remote tmux server. So they are no longer part of the VS Code tree. Do you have any ideas how to track those processes?

Perhaps you can query the actual tmux process using the pid of the terminal process?

@weinand
Copy link
Contributor

weinand commented Mar 13, 2018

Thanks, yes that was my thinking too...

weinand added a commit to microsoft/vscode-node-debug that referenced this issue Mar 17, 2018
@weinand
Copy link
Contributor

weinand commented Mar 18, 2018

Node debug (both protocols) got these new features:

  • Auto Attach mode: status bar item shows up after node-debug got activated (e.g. by running "Auto Attach" action). Only looks for node processes run in the integrated terminal that use the --debug or --inspector arguments.
    2018-03-18_19-11-26
  • Process Picker does not only show node processes but also all processes that use the --debug or --inspector arguments (e.g. Electron's helper processes). So the picker can now deal both with process ids and inspector and debug ports.
    2018-03-18_19-21-47
  • Process Picker bumps newer processes to the top of the list.
  • a new Attach to Node Process action that opens the process picker and attaches to a node process without a need for a launch config.

@auchenberg please give it a try and let me know what we should ship in March.

@weinand weinand closed this as completed Mar 18, 2018
@auchenberg
Copy link
Contributor Author

@weinand Just gave it a spin. My initial feedback:

I think the discoverability of the new auto attach button in the status bar is quite limited. I had to look for it and hover the item to read the title to figure out what it does.

To improve discoverability could we:

  1. Have auto attach enabled by default.
  2. Show a prompt to the user "We just auto attached our debugger to your node process running with --inspect/debug. Do you want this to happen going forward? Yes/No/Ask again"
  3. Store the setting as a user setting, quick users at a later point can search after to change?

I don't see the new Attach to Node Process action in today's insiders. I'll try tomorrow.

@Tyriar
Copy link
Member

Tyriar commented Mar 19, 2018

Have auto attach enabled by default.

I think there's still the performance concern on Windows of using wmic to poll the processes that can hammer the CPU.

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Mar 20, 2018

I agree with @Tyriar. Last month we made use of an npm package in the Python extension and uses wmic and we've got a few perf related issues as a result of this.

@Tyriar
Copy link
Member

Tyriar commented Mar 22, 2018

@RMacfarlane is making good progress on improvements to windows-process-tree and it should soon support everything needed by auto attach (arg list, process as a list not tree) microsoft/vscode-windows-process-tree#12.

@weinand
Copy link
Contributor

weinand commented Mar 22, 2018

@Tyriar great! I'll jump on it as soon as possible...

@weinand
Copy link
Contributor

weinand commented Mar 26, 2018

In the March release the feature is not yet on by default (and consequently there is no prompt notification) due to the performance concern on Windows. I'll keep the status bar item in the interim.

I've tried to avoid the performance issues by only starting another wmic if the previous has finished. The frequency of running wmic is determined based on the duration of the previous wmic. The idea is that wmic does not dominate the overall cpu load. In addition I only run the wmic for retrieving the "cpu load" every 4th time.

However, I will not improve the discoverability of the auto-attach feature in this milestone because there is still potential for performance issues on Windows. I'm waiting for @Tyrar and @RMacfarlane for an improved version of the vscode-windows-process-tree module.

@Tyriar
Copy link
Member

Tyriar commented Mar 26, 2018

@weinand we published the module this morning and @RMacfarlane pulled it into vscode master. So it's good to go. Note that we haven't actually tested the new APIs yet inside VS Code, only hooked up the old one which seems to work fine.

@Tyriar
Copy link
Member

Tyriar commented Mar 26, 2018

@weinand you will want to use getProcessList and pass in the ProcessDataFlag.CommandLine flag:

https://github.com/Microsoft/vscode-windows-process-tree/blob/master/typings/windows-process-tree.d.ts#L55

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Mar 27, 2018

@Tyriar Will this be available for use by other extension authors via some vscode api. I know it's an npm package, but the fact that it's got native dependencies make it difficult to package with extensions.
I'm planning on adding similar (auto attach) functionality to the Python extension, but without any dependency on wmi.

@Tyriar
Copy link
Member

Tyriar commented Mar 27, 2018

@DonJayamanne I think extensions can just import VS Code's node modules, we don't explicitly make that public API so it could break across versions though.

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Mar 27, 2018

@Tyriar

we don't explicitly make that public API so it could break across versions though.

Yes I suspected that, hence the question about it being exposed via VS Code API. A shame we cannot implement this in the Python extension due to the above limitation.
Though Node debugger will never have the same issue as its a first class citizen in VSC.

To be honest this seems a little unfair.

@Tyriar
Copy link
Member

Tyriar commented Mar 27, 2018

@DonJayamanne I pinged the team on Slack about this.

I was actually thinking about whether an API on Terminal would make sense for this to be able to request the process tree. Is that's what you're after; just the child processes on the terminal?

@DonJayamanne
Copy link
Contributor

Is that's what you're after; just the child processes on the terminal?

Yes, that would be great.

@Tyriar
Copy link
Member

Tyriar commented Mar 27, 2018

@DonJayamanne created #46737 to track

@weinand
Copy link
Contributor

weinand commented Mar 27, 2018

@DonJayamanne sorry but your statement:

Though Node debugger will never have the same issue as its a first class citizen in VSC.

is not true.

Node debugging is developed as an independent extension and the fact that it is shipping with VS Code does not mean that we are not playing by the rules.

So we are not "reaching into" VS Code for accessing node modules.

@Tyriar for the March release we (Node Debugging) will not switch to use the vscode-windows-process-tree module because there was no time to adopt it.

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Mar 28, 2018

@weinand

does not mean that we are not playing by the rules.

I didn't mean anything if that sorts.
Let me explain this better, the fact that it is bundled with vscode will always make it easier to use native dependencies than external extensions. Everytime a new version of vscode is released nodejs debugger is bundled.
Extension authors will have to worry about compatibility of older versions of extensions with newer versions of vscode (and vice versa, when using native deps). That's where nodejs debugger benefits from being bundled with vscode. Such extensions will almost never face these (version compatibility) issues.

@Tyriar thanks for considering to make this a public API.

@vscodebot vscodebot bot locked and limited conversation to collaborators May 2, 2018
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

No branches or pull requests

5 participants