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

Provide debug adapter executable the arguments for the debug session #45220

Closed
DanTup opened this issue Mar 7, 2018 · 27 comments
Closed

Provide debug adapter executable the arguments for the debug session #45220

DanTup opened this issue Mar 7, 2018 · 27 comments
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality on-testplan testplan-item

Comments

@DanTup
Copy link
Contributor

DanTup commented Mar 7, 2018

I recently merged my two debuggers and started using adapterExecutableCommand to switch between my project types to allow debugging without a launch.json.

However, while trying to support projects in sub-folders (and unit testing, which works differently to debugging on the device) I've discovered that I need to know the program and/or cwd for the session being launched in order to know which of the debuggers to launch.

Currently, adapterExecutableCommand (and it's replacement, DebugConfigurationProvider.debugAdapterExecutable) are only provided the workspace folder.

Is it possible to provide more information about the session being started? I need it to support people that have projects in sub-folders of their root and for running tests.

@isidorn
Copy link
Contributor

isidorn commented Mar 7, 2018

debugAdapterExecutable is still in preview so we could potentialy change it if we decide to.
Fowrading to @weinand since he is the author of that api

@isidorn isidorn assigned weinand and unassigned isidorn Mar 7, 2018
@isidorn isidorn added the debug Debug viewlet, configurations, breakpoints, adapter issues label Mar 7, 2018
@DanTup
Copy link
Contributor Author

DanTup commented Mar 7, 2018

It would be nice if we could bail out of this gracefully too. Currently if a user sets program to ${file} and then hits F5 after switching to a json file or something, it gets through to the debug adapter. It'd be nice if we could handle this in the UI better (show a message that the current file is not valid for launching/debugging).

@weinand weinand added the feature-request Request for new features or functionality label Mar 7, 2018
@weinand
Copy link
Contributor

weinand commented Mar 7, 2018

@DanTup your last request is covered by #44411

@DanTup
Copy link
Contributor Author

DanTup commented Mar 7, 2018

@weinand The issue I noticed was actually for when the active file was the wrong file type, not that there was no active file. I noticed it because I switched to my launch.json and added ${file} and then hit F5 again, and my debugger tried to run launch.json as a Dart script.

@DanTup
Copy link
Contributor Author

DanTup commented Mar 15, 2018

I tried to workaround this issue by making a single debug adapter that could delegate to an adapter selected at launchRequest time. However, it seems like newing up my own instance of a DebugSession doesn't work, as it's not connected to the debugger client in the same way.

@weinand Is there an easy way to "proxy" a set of DebugSessions without having to rewrite them (eg. they make super.xxxRequest calls which don't work if I created the DebugSession myself).

@weinand
Copy link
Contributor

weinand commented Mar 15, 2018

You could run the debug adapter(s) completely on your own inside the extension. See section "Run a debug adapter inside the debug extension" in https://code.visualstudio.com/updates/v1_20#_debug-api. With that approach you can do anything.

As you can see here the launching of the DA takes place inside the resolveDebugConfiguration method where you should have access to all info you need. Instead of running the DA inline you could of course spawn it as a separate process as well.

@DanTup
Copy link
Contributor Author

DanTup commented Mar 15, 2018

@weinand I was under the impression that was for debugging convenience, but is it reasonable/a good idea to do that for shipping to end users? And if so, is that API stable enough that it won't break on us?

@weinand
Copy link
Contributor

weinand commented Mar 15, 2018

There are already debug extensions out there that use this approach, e.g. Java.
What API? :-)

@DanTup
Copy link
Contributor Author

DanTup commented Mar 15, 2018

Ok cool! Now I'm wondering why we had it in another process in the first place? That caused much complication, now I'm thinking we can simplify a huge bunch of things!

@weinand
Copy link
Contributor

weinand commented Mar 15, 2018

Many DAs are not written in JS/TS but in C# or C++. How do you want to run them in the extension host?

And better make sure that your DA frees up all resources properly after a session. Running a DA for one session as a separate process is more forgiving than running it continuously as a server for a week...

And a DA that lives in an extension (and relies on the extension a lot), will no longer be able to run in any other UI frontends (because they only support DAs but not extensions).

So there are some consequences and drawbacks...

@DanTup
Copy link
Contributor Author

DanTup commented Mar 15, 2018

Many DAs are not written in JS/TS but in C# or C++

I was actually wondering about writing ours in Dart, but wasn't sure if the over-the-wire protocol was well defined and how much logic there was inside the nodejs package. Is there info on the protocol somewhere I can read up?

And better make sure that your DA frees up all resources properly

Yep, understood. We're mostly just proxying onto the Dart VM so there's only really one process for us to track, so this should be easy enough.

@weinand
Copy link
Contributor

weinand commented Mar 15, 2018

The wire protocol is defined here and the corresponding TS source is generated by this trivial tool.
Other generators seem to exist for C# and Java. You could write one for Dart ;-)

The TS library does not contain a lot of logic (see https://github.com/Microsoft/vscode-debugadapter-node/blob/master/adapter/src/debugSession.ts).

The only interesting thing might be the code that deals with the (legacy) JSON-RPC.

@DanTup
Copy link
Contributor Author

DanTup commented Mar 15, 2018

@weinand Cool, thanks for all the info. In the short-term I'll see whether moving the DA in-process solves the original problem for me.

@weinand
Copy link
Contributor

weinand commented Mar 15, 2018

Or just launch it yourself but still run it as a separate process.

@DanTup
Copy link
Contributor Author

DanTup commented Mar 15, 2018

How simple is that/ I had a look in the repo and found this doCreateProcess method but it seems to be doing a lot. Which is the correct part for me to copy/call from my resolveDebugConfig?

Thanks!

@weinand
Copy link
Contributor

weinand commented Mar 15, 2018

Noooooooo!

Just a cp.spawn("node", [ "dart-debug.js", "--server=1234" ]).

It is basically what you have in your "server" launch config.

And use a free random port (instead of 1234) and add this number to the launch config as the "debugServer" attribute.

@DanTup
Copy link
Contributor Author

DanTup commented Mar 15, 2018

Oooh, gotcha! I'll give it a go - thanks! :)

@weinand
Copy link
Contributor

weinand commented Mar 15, 2018

Did you try to run your DA "in-process"?

Just move the DartDebugSession.run(DartDebugSession); line to a separate file (e.g. like I did for Mock: https://github.com/Microsoft/vscode-mock-debug/blob/master/src/debugAdapter.ts)

and in your resolveDebugConfiguration method adapt the code from https://github.com/Microsoft/vscode-mock-debug/blob/9cdbe95adad6fbecdc85055f286f0c7391539a65/src/extension.ts#L66-L76 to this:

			// start port listener on launch of first debug session
			if (!this._server) {

				// start listening on a random port
				this._server = Net.createServer(socket => {
					const session = new DartDebugSession();
					session.setRunAsServer(true);
					session.start(<NodeJS.ReadableStream>socket, socket);
				}).listen(0);
			}

			// make VS Code connect to debug server instead of launching debug adapter
			config.debugServer = this._server.address().port;

@DanTup
Copy link
Contributor Author

DanTup commented Mar 15, 2018

It seems to launch, but fails with ENOENT spawning my VM - I wonder if it's the working directory? But I'm not sure how to set that from the code above?

By the way - why don't I have the debugServer property on my DebugConfig? I had to cast:

(config as any).debugServer

I've got my engine in package.json set to v1.21 and have run npm install to get latest, but it's not there (yet I can see your code assigning it?). I can't see debugServer in https://github.com/Microsoft/vscode/blob/master/src/vs/vscode.d.ts anywhere.

@DanTup
Copy link
Contributor Author

DanTup commented Mar 15, 2018

Actually, I'm not sure it's working - debugger never seems to recieve the launch request. This is the code I have in my resolveDebugConfiguration:

// Start port listener on launch of first debug session.
if (!this.debugServer) {

	// Start listening on a random port.
	this.debugServer = net.createServer((socket) => {
		const session = new DartDebugSession();
		session.setRunAsServer(true);
		session.start(socket as NodeJS.ReadableStream, socket);
	}).listen(0);
}

// Make VS Code connect to debug server instead of launching debug adapter.
const c: any = config;
c.debugServer = this.debugServer.address().port;
return c;

Just seems to silently do nothing when run (this code does run, but nothing happens in the test extension host - debugger toolbar never appears, etc.). I've turned on All Exceptions and Promise Rejects in the debugger, but nothing.

@weinand
Copy link
Contributor

weinand commented Mar 15, 2018

If you are running the DA in-process, it is now trivial to debug your DA as part of the extension in VS Code.

Just press "F5" in your extension and in the new window open a dart project and start debugging again with "F5".

Is your resolve method hit?
Is your DartDebugSession started correctly?
Does the returned launch config look right?
Did you try this on Windows?

@DanTup
Copy link
Contributor Author

DanTup commented Mar 15, 2018

It calls resolve, and everything looks good (except I had to do the weird cast mentioned above because debugServer is missing), however after I've stepped to the end of resolve, it never seems to hit launchRequest inside the DA and the extension host never seems to get into a debug session.

I can't see anything different to yours though.

This is the exact change:

Dart-Code/Dart-Code@c3dd0c5

I tried both with DartDebugSession and FlutterDebugSession, but see same behaviour. I'll have to try debugging more tomorrow or next week.

@DanTup
Copy link
Contributor Author

DanTup commented Mar 17, 2018

@weinand Ok, getting somewhere! My failure to launch was a typo - I had two variables, config and debugConfig and I was returning the wrong one (a wrapper over the settings!). The ENOENT was a bug I'd introduced recently (quoting calls to spawn and forgetting to set shell: true in one place).

So, everything is good except I don't understand why I need this cast and you don't:

(debugConfig as any).debugServer = this.debugServer.address().port;

My debugConfig only has name/type/request (and an indexer) but yours seems to have the debugServer property. I'm on 1.27 of debug adapter and debug protocol and 1.21 of vscode.d.ts. Any ideas?

@DanTup
Copy link
Contributor Author

DanTup commented May 1, 2018

@weinand I have no need for this now, the above solution works fine, so feel free to close. I am still confused by thing mentioned in my last comment though; I'm not sure why I get a compile error and have to cast DebugConfiguration to any because it doesn't have a debugServer property, but yours seems to.

@weinand
Copy link
Contributor

weinand commented Sep 15, 2018

In the proposed API I've added a new method provideDebugAdapter:

https://github.com/Microsoft/vscode/blob/3193667fc47265a5605bc260be38de0ad432a461/src/vs/vscode.proposed.d.ts#L479-L500

The previous method debugAdapterExecutable is still available but we plan to remove it as soon as the new method gets approved.

The new method makes it possible to run a node.js based debug adapter "inline" in the extension with this:

private _server?: Net.Server;

provideDebugAdapter(session: vscode.DebugSession, folder: WorkspaceFolder | undefined, executable: vscode.DebugAdapterExecutable, config: DebugConfiguration, token?: CancellationToken): ProviderResult<vscode.DebugAdapterDescriptor> {

	// start port listener on launch of first debug session
	if (!this._server) {

		// start listening on a random port
		this._server = Net.createServer(socket => {
			const session = new MockDebugSession();
			session.setRunAsServer(true);
			session.start(<NodeJS.ReadableStream>socket, socket);
		}).listen(0);
	}

	// make VS Code connect to debug server instead of launching debug adapter
	return new vscode.DebugAdapterServer(this._server.address().port);
}

/cc @rodrigovaras @jramsay

@DanTup
Copy link
Contributor Author

DanTup commented Oct 15, 2018

@weinand For DAs already spawning their own server in-process in the resolve method, is there value in moving that code to provideDebugAdapter?

@weinand
Copy link
Contributor

weinand commented Oct 15, 2018

@DanTup for the time being, the only value in moving that code to provideDebugAdapter is that you do not have to use the debugServer attribute to communicate the port number back to VS Code.

@vscodebot vscodebot bot locked and limited conversation to collaborators Oct 30, 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 testplan-item
Projects
None yet
Development

No branches or pull requests

3 participants