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

Proposal: Allow multiple debug sessions through a single adapter #79

Closed
EricCornelson opened this issue Oct 21, 2019 · 65 comments · Fixed by #326 or #344
Closed

Proposal: Allow multiple debug sessions through a single adapter #79

EricCornelson opened this issue Oct 21, 2019 · 65 comments · Fixed by #326 or #344
Assignees
Labels
feature-request Request for new features or functionality
Milestone

Comments

@EricCornelson
Copy link

EricCornelson commented Oct 21, 2019

Currently, both Visual Studio and Visual Studio Code both support multiple concurrently running debug sessions, and Visual Studio Code supports hierarchical debug sessions which can represent debugging child processes or multiple "global objects" (example).

In this vein, there are some scenarios where it would be useful to be able to have multiple debug sessions running through a single debug adapter, which would allow for some more advanced orchestration between debug targets.

A particularly interesting example of this is when debugging a web page which has associated web workers and service workers. The Chrome DevTools protocol allows for the concept of a "flat session" debug mode, which allows a debugger to simultaneously debug all related pages and workers at the same time through a single connection using a "targetId" to route messages to the correct debug target. Additionally, it looks like this will soon become the default in Chrome DevTools, and the "non-flat" session mode will be deprecated (see crbug.com/991325).

In the above example, debugging a web page through a single "flat session" confers many benefits (e.g. being able to start each child target in a suspended state to allow the debugger to configure breakpoints and other set-up before the child starts running). However, running in "flat session" mode requires debugging these multiple targets through a single connection, something which, today, is hard to do using built-in functionality in the debug adapter protocol (it can be achieved by running in-process in VS Code and leveraging the DebugAdapterDescriptorFactory to share state between sessions, but that solution is clunky, and specific to the VS Code API, and therefor does not translate well to Visual Studio, for example).

Some other examples of scenarios where a "flat session" mode might be useful are:

I've created a PR which, as a first step, adds an optional "sessionId" parameter to each protocol message. If this sounds reasonable, I think it would also be nice to give adapters a way to notify the host that a new child session has been attached, as right now an adapter has to manually call VSCode or Visual Studio APIs to launch a new debug session and then trap it on entry to achieve this.

Let me know what you think!

@roblourens
Copy link
Member

I like this. Does the protocol need to think about how a client knows whether to route a new request to the same adapter vs creating a new one, or is that up to the client?

@EricCornelson
Copy link
Author

To start with, I think we could leave it up to the client. On the VS Code side, it's pretty easy to leverage the DebugAdapterDescriptorFactory to trap new launches and re-use the same adapter instance. On VS that's a little harder, since there's no similar built in "factory" interface, but since adapters using the PineZorro extension have the ability to handle launching themselves, it's fairly easy to just use that to route everything back through the same adapter instance (which is what we're doing right now).

If we end up finding this model useful outside of javascript debugging, it would be nice to have a way to tell the host that we want to re-use the same adapter instance in "flat session". For that we could just use the capabilities event and publish that an adapter can support flat session mode.

@connor4312
Copy link
Member

connor4312 commented Oct 23, 2019

This sounds neat. I like the intention and it'd certainly make some scenarios easier as you mentioned. Stepping between the client and service is super interesting, being able to step through fetch() into an Express route and back out again would be neat. In the (longer term) future it might be useful to have a contribution model in vscode-pwa to give intelligence around frameworks like that, beyond the generic logic we have out of the box...

In implementation we may want to have a new preflight request prior to launching, for the client to confirm whether it is able to attach a session to the existing adapter. Adapters won't all support flat sessions for some time, and then there might(?) be scenarios where one adapter is unable to flatten requested sessions into its existing instance. A graceful rejection or a 'method not found' error would cause a new instance of the adapter to start.

@weinand weinand self-assigned this Oct 24, 2019
@weinand weinand added the feature-request Request for new features or functionality label Oct 24, 2019
@weinand weinand added this to the On Deck milestone Oct 24, 2019
@weinand
Copy link
Contributor

weinand commented Oct 24, 2019

@EricCornelson Thanks for this interesting proposal.

My first reaction:
This feature sounds like an optimisation for functionality that already exists.
Stepping between client and server code is daily business for VS Code and it is supported for all combinations of client and server languages (e.g. TS in browser and Java and Python in the backend servers).

With this "optimisation" a single debug adapter can be used to handle multiple sessions if these sessions use the same runtime. This makes sense for a CDP-DA but I'm not aware of many other runtimes that would want to support this.

On the other hand I see quite some VS Code restructuring work necessary to support this feature.

So before we want to commit to this effort, you must convince me that there are no alternatives. ;-)

One alternative is what the Java extension is currently doing: they are running a single DA process that serves multiple debug sessions through different socket connections. In the DA server all sessions are using the same underlying runtime and/or debug target connection.

So this approach already "Allows multiple debug sessions through a single adapter". But instead of multiplexing via a new "sessionId" attribute, communication channels of the OS are used.

Would this solve the issue you are trying to address?

@int19h
Copy link

int19h commented Oct 24, 2019

The Python adapter is using this model - a single adapter managing several related concurrent debug sessions, with multiple connections to it from VSCode via sockets - for multiprocessing scenarios (although we are still working on that implementation). Having a single channel to talk to the IDE would be marginally simpler on the adapter side, but it's not really a big deal.

One related thing that would be nice to have is some way for a session to tell the client to create another session (i.e. effectively, issue a reverse "attach" request, that would result in creation of a new session with the supplied debug configuration). We're using a custom event for this, and it works... but it's not a client-agnostic solution, and requires re-implementing the client part for every IDE. Since debugging multiple related processes is a generic scenario, not really tied to any particular language or IDE, and it needs this bit for the adapter to orchestrate things on the client, it would be nice to have that supported in the core DAP protocol.

@EricCornelson
Copy link
Author

@weinand We actually started our proof of concept using sockets, but the big draw for running everything through a single communication channel is remote debugging in VS. Multiple socket connections are fine for local debugging, and probably even debugging a remote workspace in VS Code, but if the user is debugging a remote process either in a VM or a server, and their workspace is local, it gets trickier.

We currently have a way through VSDebugAdapterHost and msvsmon to launch the debug adapter process on the remote machine, where the adapter can then launch the desired program and start debugging, and we communicate with the adapter process via its stdin/stdout through an SSH tunnel. But we can't open a socket connection to a debug server process on the remote machine without either manually doing some port-forwarding manually or dealing with firewall issues (that is, there's nothing built in to VS that would make this easy for us, as far as I know).

@andrewcrawley pointed out that if we wanted to go the debug server route, we could do something like launch the first adapter as a debug server, and then for each child debug session in VS launch a "proxy" process that would make the connection to the debug server and then pipe the traffic back through its stdin/stdout. While that could theoretically work, we both agreed that this was a less-than-ideal workaround, as it involves a lot more moving parts, and has more potential for problems to arise.

I'm not sure how that particular scenario translates to other adapter projects, but it was the biggest factor for us to go with the "flat session" communication model.

@int19h +1 We are doing the same thing RE: the custom "reverse attach" event on the VS side, and have had the same thoughts. I didn't add that piece to the proposal, but maybe that's a better place to start. This is where we are passing the sessionId for each new child session back to the host as well, and my thought is that we're essentially telling the host "we are already managing this child debug session, just letting you know".

I'm curious, though, if we added a "reverse attach" event to the protocol, how would hosts be expected to handle that event? Right now, what we're doing with vscode-pwa is we manually send a new "start debugging" command and trap the launch to route it through the already-running adapter. But if we consider that "flat session" is not a thing in the protocol, what should the host do if it gets one of these events? We are essentially notifying the host so it can display the session in the UI and start sending commands to it. So then the question becomes, well how does the host start sending commands to this new session? We could give it a port to connect to, but I'd assume we wouldn't want to force a specific implementation (sockets) for new child sessions, and instead leave that up to the adapter implementation. Just firing a new "start debugging" command doesn't seem to make much sense to me either, because the session is actually already running in the current adapter, right, we just want to connect to it? If we fire a new "start debugging" command, then we are still in the business of having to trap the launch and route it through our existing adapter, either in a DebugAdapterDescriptorFactory for VS Code, or some other IDE specific piece, or we are again forced in our implementation to use sockets (which is not always desirable, as in the scenario I mentioned above).

So that leads me back to having a "flat session" option, where you can tell the host, "hey, I've connected to this new child session, please represent it for me in the UI and start sending messages to it over the existing connection with this identifier". 😃

I definitely understand that it doesn't make sense to add something to the protocol if it only fits a very narrow use case (especially if there is a workaround), which is also the main reason I drafted up the proposal. I was curious to see if there was any other adapters out there that might benefit from a "flat session" mode, and it sounds like there is at least one other!

@int19h
Copy link

int19h commented Oct 25, 2019

What we do right now is have the adapter send the complete debug configuration in the reverse attach request. The custom event handler in VSCode then starts a new debug session with that configuration. The adapter creates it by remembering the config it was passed in the request for the initial session, and then just changing it to "attach" (if it wasn't already), and adding a property to designate which known debuggee to attach to (by PID in our case).

On the VSCode side, we use DebugAdapterDescriptorFactory to make connections go directly to the socket, without spawning another adapter. If the primary connection was "launch", we assume that the IDE is running on the same machine, and thus those secondary connections can always use 127.0.0.1 as hostname. And for the port, we start a listener in the ephemeral port range, and then retrieve the actual port, and pass it in the reverse attach request. If it was "attach", then the original debug config already has the hostname and the port as properties, and they just get reused as is.

For remote scenarios, this means that the other machine only needs to have that one port opened. This can then be tunneled via SSH for security purposes - for now, we tell the users to do this manually, but it would certainly be nice to have SSH as a built-in DAP transport directly in VSCode, with a single shared config for all debuggers that need it.

So there are really two pieces here that require custom IDE-side code right now. One is the custom event handler to create a new session, and the other one is the adapter factory that directs the DAP connection for that to go over a socket. VSCode kinda sorta has the latter in form of "debugServer" already, but since there's no way to specify the hostname, it's only useful for local debugging, not remote. I forgot about that second part, because we use this approach for all "attach" sessions, not just secondary ones, so we implemented it outside of the multi-session scenario.

And yes, you're right - to standardize this in DAP would require it to be aware of the transport layer, since the client can't just spawn adapter to service the reverse request to start a new debug session. Which means that "debugServer" would have to be added to the spec for "launch" and "attach" requests (and extended to accommodate hostname in addition to port), and clients supporting it would be required to implement DAP-over-TCP specifically.

So I guess it depends on how the DAP team views the protocol - is it meant to be entirely transport-agnostic in all respects, or is it accidental and can change if requirements demand it? I think there would be some value in having something as common as TCP be explicitly standardized as a transport. Especially since it'd be opt-in for the clients anyway - it's more about having them interpret the connection information in the same manner if they choose to support it.

The users would likely also appreciate it, since right now adapters that support remote "attach" over TCP don't really have a single consistent format for debug configuration for that, and each has to document it separately. If it's standardized, it could all go straight into the VSCode generic debugging documentation, where users generally land first when they start looking. And eventually, this would allow for SSH tunneling to be transparently added on top.

@mickaelistria
Copy link

As far as I can see, the multiple session approach is documented in the overview and implemented (with custom messages) in vscode-js-debug.
However, the specification doesn't specify how multiple sessions are supposed to work: how client can start a session, pass a port to use for the child session, which operations (initialize? launch?) need to be sent to the child session...?
As the behavior in vscode-js-debug seems satisfactory, can the related operations and flows be specified?

@gfszr
Copy link

gfszr commented Jul 8, 2022

Any news regarding this issue? Adding support for multi-session debugging for DAP is rather important, and each debug adapter implemeting its own vision of multi-session support creates a lot of fragmentation.

@weinand
Copy link
Contributor

weinand commented Jul 9, 2022

Today multi-session debugging happens outside of DAP because that use case must be supported anyway: if I want to debug an application that has processes implemented in C++ and Python, then two debug adapter types are needed and the client (e.g. VS Code) establishes independent sessions with these adapters via debug adapter factories that exist per debug type ("cpp" and "python").

The same approach is used when debugging multiple processes implemented in the same language (where only a single debug adapter type is needed): the client uses a single debug adapter factory to create sessions and it is an implementation detail of the factory (and the debug adapter) whether a single server process is used to implement multiple sessions via multiple communication channels or whether an independent debug adapter process is launched for each session.

With this approach the client (e.g. VS Code) does not have to deal with debug adapters that are able to handle multiple sessions. It just creates a DAP session via a factory and does not have to care about multiplexing DAP sessions over a single comm channel.

@gfszr
Copy link

gfszr commented Jul 9, 2022

@weinand Thank you for the quick reply! 🙂

Today multi-session debugging happens outside of DAP because that use case must be supported anyway: if I want to debug an application that has processes implemented in C++ and Python, then two debug adapter types are needed and the client (e.g. VS Code) establishes independent sessions with these adapters via debug adapter factories that exist per debug type ("cpp" and "python").

Considering DAP as the "generalization" of a debugger API, various debuggers (pydevd and gdb, for instance) allow the concept of "debugging child processes of the same language". However, neither pydevd nor gdb implement a feature of debugging child processes written in arbitrary languages (Well, maybe gdb does :P). If DAP's purpose is to generalize debuggers through a single protocol and session, then one of such features should be supporting debugging same-language child proccesses.

Multi-language debugging might sound similar to multi-session debugging, but the latter is a basic feature of a debugger (Thus, should be represented in DAP in some way), while the other is a fancier feature that is out of scope to a debugger (For which, VSCode can solve by using multiple DAP sessions).

With this approach the client (e.g. VS Code) does not have to deal with debug adapters that are able to handle multiple sessions. It just creates a DAP session via a factory and does not have to care about multiplexing DAP sessions over a single comm channel.

Such approach has disadvantages, which I believe DAP was designed to solve: for each debug adapter, the client would need to implement its notion of "multi-session", if one is multiplexing on the same connection or creating new connections. Such example is shown in both debugpy and vscode-js-debug, which both implement a different approach which is not only non-DAP, but also requires changes in the UI (VScode, or any other editor using DAP for that matter).

I'm a huge fan of DAP, but lacking a feature a lot of debuggers implement (multi-processing), defeats the purpose of not reinventing the wheel and write a compatability layer between a DAP server and its UI.

@puremourning
Copy link
Contributor

puremourning commented Jul 9, 2022

The main issue with leaving this to the client is that only the Server knows when a new 'target' has become available and how to connect to it. This can be demonstrated by the status quo of de facto implementation in debugpy and vscode-js-debug. both of which use (different, naturally) custom messages and custom client-side code.

I think the request is to recognise that use case and standardise a mechanism to achieve it rather than each dap sever crating a custom one and each dap client having not have per-server custom code.

In the case of vscode-js-debug, the adapter is not even basically functional without implementing its custom undocumented protocol extensions.

@weinand
Copy link
Contributor

weinand commented Jul 9, 2022

I suggest that someone creates a concrete spec proposal for a new DAP target event.

@gfszr
Copy link

gfszr commented Jul 9, 2022

@weinand I'd like to thank you again for your quick responses!

I suggest taking a similar approach to vscode-js-debug's flatSessionLauncher.ts plus the reverse attach of debugpy, and formalizing everything our fellow colleagues stated above:

  • The initial DAP session between a UI and a debug adapter is defined as the root session. A root session behaves like a regular DAP session.
  • Each session has a session ID, which is a randomly generated UUID which is created upon session creation by the server. The root session has an empty UUID.
  • At any time, the server can send an event of type newSession, and such created session is called a "child session".
{
    "event":"newSession"
    "body": {
         "configuration": {
             <Debug configuration that the UI is responsible for negotiating in the newly created session>
         }
         "sessionId": <SessionID of the newly created session>
    }
}
  • Communicating with the child session is performed on the same transport as the root session, but each packet is appended with an initial field debugSessionId, which states the session ID of the packet. If the packet does not contain a sessionID, then the packet belongs to the root session. Otherwise, the packet belongs to the session with debugSessionId as ID.
  • The UI, upon a newly created session, starts a DAP negotiation with the new session using the configuration supplied in the newSession event.

As stated above, such approach would have a lot of benefits:

  • Support by older UI's and implementations that ignore the debugSessionId in packets, and ignore the newSession event. This can also be negotiated in the capabilities in debug adapter startup.
  • Transport-agnostic - as all transportation with child sessions are multiplexed in the same origin DAP session, DAP would continue to be transport-agnostic.
  • Minor UI changes - UI's which desire to do support multi-sessions (Or already support, such as VSCode), need only to add support for accepting such event and creating a new DAP session that is being multiplexed on a previous DAP session.
  • Multiple session support over a single transport is good for remote scenarios, which become more and more common.

One downside for such approach that I thought of is the root session transport being congested by many child sessions being multiplexed (locking, etc.). However, DAP doesn't generate a lot of traffic, so I do not believe this would cause issues.

Please feel free to comment and advise on the suggestion above 👍

@connor4312
Copy link
Member

Pavel noted rightly that there are two things here: the "reverse attach" request -- a request from the DA for the client to start a new debug session, and the specifying how the communication happens.

I think the reverse attach is sensible and fairly simple: a new request like startDebugSession that contains a launch configuration that the DA wants the client to start. This can be done without particular knowledge of hierarchy or session reuse.

The notion of hierarchy could be brought in through an child: boolean property on this request. If the hierarchy in this proposal functions as hierarchal sessions do in VS Code today, the effect of child: true is purely cosmetic and could in fact be ignored.

The request could additionally contain a connectionId: number that, if present with child: true, indicates the DA would like the UI to reuse the same connection/debug server. I would suggest using Eric's proposal, but renaming it to connectionId to avoid confusion, as sessionId is already in use.

Why not make it be the real "session ID"? (Which is not a concept of DAP itself, only one from VS Code.) My reasoning is that the session ID should not be assigned by the DA, but also that having it being returned from the DAP call introduces additional ordering complexity, since it must be returned before the new connection is made so that the DA can treat it correctly. We can remove the complexity by having the DA assign an integer, which is also more compact on the protocol than a UUID for instance.

With this, the "root" debug session would not have a connection ID passed to its calls, but children would.

interface StartChildSessionArguments {
	configuration: {
		type: string;
		request: string;
		[key: string]: unknown;
	},

	child?: boolean;

	connectionId?: number;
}

This would also need a corresponding supportsStartChildSession in the InitializeRequestArguments.

@gfszr
Copy link

gfszr commented Jul 10, 2022

@connor4312 Very nice!
I do have a few comments:

  • Why not always reusing the same transport? Although it might complex the DAP server because it requires it to implement multiplexing (rather than suggesting it), it will save the hassle that every adapter would have to have some configuration allowing the user to make sure connection reuse is used (For remote scenarios, for instance), so basically all DAP servers would have to support some way of multiplexing, so why not always multiplex?

  • Regarding session ID and ordering issue - the adapter needs to have some way of mapping a newly created DAP session to an actual child process that is going to be debugged. Such mapping can be created from either:

    • The new session's ID, which is appended to each multiplexed packet. This makes it easy as the adapter accepts such multiplexed connection only if the adapter already added the session/connection ID to the list of known ID's before returning the StartChildSession request.
    • The configuration supplied in Arguments. This means the server must always accept multiplexed connections in any
      ID, and figure out the actual debuggee after configuration is sent.

    I believe that the former approach is eventually easier to implement, and the ordering issue should be of no concern, as the adapter can perform any state update on its side before sending the StartChildSessionRequest.

  • I agree that ConnectionID being a small integer is better, but that ID should still be server generated, as it will make the UI's easier to implement without having to worry about ID reuse.

@mickaelistria
Copy link

I don't think we need to deal with sessionId.connectionId or anything of that form as the spec already tells a bit about how things can work. It just misses some technical aspects to actually implement it.
The only thing I think is missing for the spec to properly specify multi-sessions so that all adapters/clients can support it is agreeing on a port for a child session. According to the spec For every debug session, the development tool initiates a new communication session on a specific port, good. Then what we need is a way to know how to initiate a new communication session on a specific port. How to have the client and adapter agree on a port? Should the client pass the port as a parameter it in Initialize, or Launch/Attach? What is the first command the client must sent to the adapter in order to start a new session? I don't think a new custom command to start a session is necessary, do we?
@weinand If you agree with this overall idea and can recommend what is best, I'll happily try to turn it into a concrete proposal as a PR.

@puremourning
Copy link
Contributor

The client also needs to know the initialisation options and various other things, as included in @connor4312's proposal. But remember that the purpose of this particular proposal is to allow multiplexing sessions on a single transport, rather than requiring that all adapters working with multiple sessions use IP for transport. This is (presumably) based on the existing (de facto) implementation in vscode-js-debug (which I understand is actually used by Visual Studio (not code)), according to its author.

But I tend to agree that this multiplexing is an additional complexity that's not required for the spec to be functional - it's required for specific clients to be functional. Therefore I tend to agree with @mickaelistria that we only need to specify a ChildDebugSessionLaunched notification, which needs at least:

  • hostname/port to connect to
  • the launch/attach request type
  • the contents (used verbatim) of the launch/attach request arguments

I believe that this is pretty much exactly what debugpy does.

Example:

Name: childSessionLaunched
Type: Notification
Arguments:
  connection: 
    hostName?: Address to connect to (local host used if not supplied)
    port: port to connect to (per multi-session specification today)
  request:
    type?: "launch" or "attach" (default: "attach"?)
    configuration: [ object containing the launch/attach arguments passed to the adapter  ]

I don't believe that anything needs to be specified wrt to initialise exchange, there's an interesting question about breakpoints (should the client re-send all breakpoints to the new child), but I think that's up to client implementations to decide what works best. Perhaps a future update could include a "file type" or something which might indicate to the client that the child session is in a different language, but that seems rather scope-creepy at this point.

I think if we had that (or similar) in the spec, then there would be no need for these connectionId or sessionId, unless there is a specific use-case where IP is not ideal. I can think of a few reasons where that might be the case:

  • security (opening a local port with a debugger listening on it could be opening a vector); a single (multiplexed) stdio 'transport' is not prone to this
  • portability (solving the security issue can be done with domain socket/named pipe, but these are not trivially portable)
  • routing - ip routing and firewall may be an issue; I may have only opened 1 port into my docker container, for example, and now I must open an arbitrary number of them, requiring me to negotiate with my debug adapter to restrict ports to a range).

Well, having written all of that, the multiplex-over-stdio does seem to have some key advantages.

As the existing multi-session support is already in the specification, I'd be tempted to add both approaches and let the client decide which it wants to use by capabilities. The above childSessionLaucnhed request can be extended with a connection type:

Name: childSessionLaunched
Type: Notification
Arguments:
   sessionId?: if supplied, multipllex on the current session. connection block must not be supplied
  connection: 
    hostName?: Address to connect to (local host used if not supplied)
    port?: port to connect to (per multi-session specification today)
  request:
    type?: "launch" or "attach" (default: "attach"?)
    configuration: [ object containing the launch/attach arguments passed to the adapter  ]

We can of course include the child property for the reasons @connor4312 includes, though I'm not sure when this would ever be false in practice. I don't think the 'hierarchy' aspect is unique to either launch approach. I think the only advantages of the multiplexing over the existing multi-session are the security, portability and routing stuff above.

TL;DR - agree mostly with @connor4312's proposal, but (probably) for different reasons.

@gfszr
Copy link

gfszr commented Jul 11, 2022

@puremourning Although de facto in current implementation of debugpy, using a different transport (Especially a concrete typed one, such as IP+Port) than the current session isn't something we should encourage. Standardizing multiplexing for supporting child sessions, although more complex but not by much, will eventually benefit the user and the client developer, not only from the very good reasons you specified but:

  • Suppose a new type of transport is added to the world which we want DAP to operate on - would we need to add another support in childSessionLaunched for this type of transport to support it?
  • Less "transports" that both the DAP server and client should manage - less sockets/named pipes/SSH tunnels.

If a DAP server doesn't want to implement multiplexing support - it's OK, it can disable support for childSessionLaunch handling in the capabilities which will backtrack to the current experience. However, multiplexing will be - by far, the best option.

@puremourning
Copy link
Contributor

puremourning commented Jul 11, 2022

The existing (spec'd) multi-session approach isn't going to disappear, not least because it's very convenient for remote debugging, etc. So clients have to implement that anyway; I don't see any significant extra complexity on the client side.

Speaking as the author of a client, in my client multiplexing is more challenging than completely separate comms.

@weinand
Copy link
Contributor

weinand commented Jul 11, 2022

I don't like the fact that in this feature request "starting new DAP sessions from within a debug adapter" and "multiplexing DAP sessions on a single transport" are mixed together. The latter is just an optimisation which does not result in any architectural benefits, whereas the former is an important architectural feature because it allows debug adapters to handle multi-session debugging without need for non-standard code in VS Code debug extensions.

If we combine both sub-features into one, I see the problem that a client like VS Code will have a hard time to adopt the full feature because it will require significant changes in lots of places. This means that we end up with a DAP feature that DAs can already use, but which will not be supported by VS Code for quite some time (and therefore still requires the existing approach as a fallback).

If we could agree on separating the "multiplexing" then we could focus on the "starting new DAP sessions from within a debug adapter" which I believe will result in a fairly small change in clients (including VS Code).

But the tricky part here is to avoid talking about concrete communication channels like "ports" and "hosts". Remember the debug adapter factories available in VS Code's extension APIs allow for sockets, named pipes, and even inline implementations that do not use a communication channel at all because the DA is fully implemented in the extension...

@mickaelistria
Copy link

But the tricky part here is to avoid talking about concrete communication channels like "ports"

The specifically already tells about port in https://microsoft.github.io/debug-adapter-protocol/overview#debug-session-start . Are you willing to roll back that part for something more abstract? I personally wouldn't mind if we stick to ports, it's standard enough to be usable for all clients and adapters and pragmatically efficient.

@weinand
Copy link
Contributor

weinand commented Jul 11, 2022

@mickaelistria port does not occur anywhere in the spec because the spec does not cover how a DA is launched and how a communication channel to the DA is established.

For security reasons some debug extension authors refuse to use ports and rely on namedPipes... And I don't think that we can add concrete communication mechanisms to the spec.

@zobo
Copy link
Contributor

zobo commented Sep 9, 2022

A bit late to the game, but here are my thoughts.

Looking from my own perspective (php-debug) the most important thing is how the DA is spawned from the client (VSCode). Since in my case the first DA instance will listen for incoming TCP connections from PHP/Xdebug and then create a new DA instance for each of them. This means either all of the DAs need to run in the same process (be it VSCode itself or one DA host process) or do something very creative with fork() or some TCP proxying - feels too complex.

Also, I feel that for my use case the user experience is better if the "child" debug sessions are actually NOT hierarchically under the primary "listener" debug session. This way the user can "stop listening" but continue to debug the open sessions.

I understand that this proposal tries to unify how different clients offer creating a new session from a DA instance, but with all the "implementation detail" out of scope we'll see how it will work in VSCode and then across other clients.

Great first steps, happy this is moving along.

@connor4312
Copy link
Member

Since in my case the first DA instance will listen for incoming TCP connections from PHP/Xdebug and then create a new DA instance for each of them. This means either all of the DAs need to run in the same process (be it VSCode itself or one DA host process) or do something very creative with fork() or some TCP proxying - feels too complex.

I'm not totally clear on what would change for you in this mode. Looking at php-debug, you use the program attribute in the package.json, so VS Code will run that Node program for each new client session. If you use the new startDebugging request, that will continue to happen. Am I misunderstanding your scenario?

@connor4312
Copy link
Member

Also, I opened the proposal for multiplexing here: #329

@zobo
Copy link
Contributor

zobo commented Sep 9, 2022

I should have been more concrete. What I am talking about is not in the main branch, but a separate feature branch, not yet merged.

Here I use the InlineDebugAdapterFactory so that all DA instances run in the same process and can share data in static properties...

What I'm trying to say is that startDebugging will not be enough for some extensions without being able to control how DAs are spawned. I'm looking at your other proposal and this could probably be a good start.

Maybe a flag in package.json could be used to set a spawning mode like:

  • stdio
  • tcp, new for each
  • tcp, reuse for child session
  • tcp, one instance only
  • ...

Or a filed in the startDebugging reverse request...

Keeping this consistent across different clients could be a problem...

@jonahgraham
Copy link
Contributor

Can #333 be looked at before 1.59 is finalized?

@roblourens
Copy link
Member

Due to #333, I'll revert this change in main, and we will finalize it in October. I'd like to implement it in vscode during October.

@roblourens roblourens reopened this Oct 4, 2022
roblourens added a commit that referenced this issue Oct 19, 2022
roblourens added a commit that referenced this issue Oct 20, 2022
* Add startDebugging request
Fix #79

* Remove whitespace

* Update specification.md

Co-authored-by: Mathias Fußenegger <mfussenegger@users.noreply.github.com>

* Fix wording

Co-authored-by: Mathias Fußenegger <mfussenegger@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment