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

Allow extensions to handle launching of the debug adapter. #8645

Closed
vadimcn opened this issue Jul 1, 2016 · 17 comments
Closed

Allow extensions to handle launching of the debug adapter. #8645

vadimcn opened this issue Jul 1, 2016 · 17 comments
Assignees
Labels
api debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality
Milestone

Comments

@vadimcn
Copy link
Contributor

vadimcn commented Jul 1, 2016

It would be nice if extensions could be in the loop when launching the debug adapter sub-process:

  • This would allow the extension to alter adapter's command line options depending on workspace configuration and/or pass the relevant bits of configuration to the adapter.
  • In cases when adapter is implemented on top of system debugger, this would allow the extension to locate a proper binary (for example, lldb could be installed as lldb.X.Y on some systems).
  • Extension could provide better error messages should the adapter exit abnormally.
@bpasero bpasero added feature-request Request for new features or functionality api debug Debug viewlet, configurations, breakpoints, adapter issues labels Jul 2, 2016
@weinand
Copy link
Contributor

weinand commented Aug 9, 2016

@isidorn this feature request makes a lot of sense and would bring debug-adapters closer to language servers.

@isidorn isidorn added this to the September 2016 milestone Aug 23, 2016
@weinand weinand removed this from the September 2016 milestone Aug 29, 2016
@isidorn isidorn added this to the Backlog milestone Sep 20, 2016
@isidorn
Copy link
Contributor

isidorn commented Oct 17, 2016

Setting to November to investigate

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Oct 20, 2016

I agree this would be a useful feature. As I've found people using the Python extension and attempting to start the debugger with invalid launch configuration settings (basically the path to the python interpreter is incorrect).

@DustinCampbell
Copy link
Member

From a scenario-perspective, this is related to my request in #10861.

@isidorn
Copy link
Contributor

isidorn commented Nov 2, 2016

Suggestion: a debug extension can contribute a "launch" command in its 'package.json' - "launch": "extension.node-debug.launchAdapter". If this contribution is provided vscode will not launch the debug adapter but will trigger the provided command instead.

This command should launch the adapter on the extension side and return the port to which vscode will connect.
If the adapter is launched in this way the extension needs to take care of the whole lifecycle of the adapter - thus it needs to shut it down once vscode sends the 'disconnect' request.

@weinand let me know if this makes sense to you

@weinand
Copy link
Contributor

weinand commented Nov 2, 2016

@isidorn one complication: VS Code does not connect to a debug adapter through a socket but uses stdin/stdout.

@isidorn
Copy link
Contributor

isidorn commented Nov 2, 2016

@weinand I think this should be fine as it is already covered in the case when a "debugServer"is specified in the 'launch.json' and vscode just connects to the running instance. You can check out the code here

@weinand
Copy link
Contributor

weinand commented Nov 2, 2016

@isidorn no, this is not really covered because debugServer mode is a special development mode, it was never meant to be used for production. E.g. port management is manual, and life cycle management of a debug adapter is different in server mode.

@gregg-miskelly
Copy link
Member

Sorry if this is obvious, but isn't the best answer here to provide the marshalling support for marshalling the pipe handles used to communicate with the debug adapter from VS Code->node language service->debug Adapter. In other words, some sort of API that takes stdin/out as some sort of handle, and then, if needed, a 'create process' API for spawning a process that handle those handles?

@weinand
Copy link
Contributor

weinand commented Nov 3, 2016

@gregg-miskelly 'stdin/stdout' (in the Unix sense) is a communication mechanism that only works between a parent process and its child, so there is no 'handle' that can be passed to another process. Or in other words: stdin/stdout is not a named pipe which can be easily sent to another process.

I see two options here:

  • debug adapters that need to be launched from an extension need to support named pipes as their communication mechanism.
  • the debug adapter is still launched from VS Code (so that stdin/stdout can be configured) but the extension gets a chance to 'calculate' everything that is needed for the launch (e.g. executable, args, and environment).

@vadimcn
Copy link
Contributor Author

vadimcn commented Nov 3, 2016

AFAIK, stdin/stdout are just like any other file descriptor, so they could be passed between processes. This requires using Unix domain sockets though, and VSCode probably does IPC over pipes?

Another option I can think of, is to allow an extension to be the debug adapter. After all, why does VSCode insist on debug adapters being their own processes, when everything else can be done in-process? If, for whatever reason, an extension prefers to spawn a new process for hosting the adapter, it can always do that, right?

@weinand
Copy link
Contributor

weinand commented Nov 3, 2016

@vadimcn introducing a Unix domain socket as a secondary communication mechanism between the VS Code renderer process and the extension host process just to be able to pass file descriptors around sounds awkward. And for Windows we would need yet another approach. So I don't consider this a viable solution.

And your second option doesn't work either: the extension host is a single node.js process running JavaScript whereas debug adapters can be implemented in any language. Their life cycle and privileges are different than the extension host's life cycle and privileges.

@vadimcn
Copy link
Contributor Author

vadimcn commented Nov 3, 2016

I agree on the first point. Just pointing out that passing fds is possible.

Not sure what you meat by "privileges". Is there anything special going on regarding debug adapters in terms of security? I thought they run under the same user token as VSCode itself?

As for the rest, I meant that debug adapters could have been just another type of JavaScript object inside an extension, exposing an API for interacting with them. Internally, they'd be free to spawn an external process, if that's required, and proxy messages to/from it.


But it's probably too late in the game to change all that, so how about this:

  • VSCode creates a named pipe and asks extension to start a new debug session, passing it name of the pipe.
  • Extension opens client side of the pipe, then spawns a debug adapter passing pipe fd as stdin/stdout.
  • VSCode can now communicate directly to the adapter.

@weinand
Copy link
Contributor

weinand commented Nov 3, 2016

@vadimcn if a debug adapter needs to run in elevated mode (e.g. suid mode), we do not have to run the extension host in that mode.

Yes, debug adapters could have been designed as a JavaScript based proxy running inside the extension host. But since our first debug adapters were implemented in C# and no extension host existed at that time, it was neither obvious nor natural to introduce a JavaScript based proxy server as the design center. Instead we came up with a wire protocol (instead of API) based language-neutral approach. This approach makes it possible to run debug adapters in alternative clients that do not even use an extension host.

Your proposal from above is a variant of my proposal which avoids that the debug adapter has to deal with the named pipe directly. That's a good idea, we might try that.


If I read your initial comment again, it seems that even my second option from #8645 (comment) would work for you, right?

@vadimcn
Copy link
Contributor Author

vadimcn commented Nov 3, 2016

If I read your initial comment again, it seems that even my second option from #8645 (comment) would work for you, right?

It would, for the most part.

My extension also opens a secondary channel to the adapter - so that I can have keyboard-bindable commands, that e.g. switch the default display format of variables. It'd be easier to establish this channel if extension were a direct parent of the adapter process. Right now I pass a port number via a file in /tmp...
Of course, this can be handled in other ways, for example, by modifying debug protocol to enable passing messages between extensions and adapters.

@isidorn isidorn removed their assignment Nov 15, 2016
@weinand
Copy link
Contributor

weinand commented Dec 5, 2016

@vadimcn with #15656 it will become possible to use the debug protocol (and private extensions to that protocol) straight from the extension host. This could alleviate the need for opening a secondary channel to the adapter.

@weinand weinand modified the milestones: January 2017, November 2016 Dec 5, 2016
@dibyendumajumdar
Copy link

Hi @weinand

I see two options here:
debug adapters that need to be launched from an extension need to support named pipes as their communication mechanism.
the debug adapter is still launched from VS Code (so that stdin/stdout can be configured) but the extension gets a chance to 'calculate' everything that is needed for the launch (e.g. executable, args, and environment).

I have been trying to work out how to support different versions of Lua in my debug adapter. Being able to pass details that allow the extension to calculate what needs to be launched would be welcome. But is it also possible to allow a simpler 'version' selector for launching the extension?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

8 participants