Sync breakpoints outside of debug sessions

[SeeminglyScience]

PR Summary

This change is paired with PowerShell/vscode-powershell#4065

Adds a client utilizing a custom notification to sync breakpoints between the client and the server at all times. All breakpoint additions, removals, and enable/disable will be synced regardless of if they occur within the console (e.g. sbp -Variable someVar -Mode Write) or in the UI. The only exception is when a breakpoint is enabled/disabled in the PSIC (there's no way to directly enable or disable a breakpoint in the vsc API afaict)

Setting as WIP as I still need to figure out a way to add tests, but please review carefully anyway @andschwa. Ideally we'd release a stable before merging this as I'd like a slightly longer preview period for this change if possible.

[JustinGrote]

Hey @SeeminglyScience and @andyleejordan I can probably go thru and clean up conflicts to get this up to date, but I was wondering what is the impetus? Is there a limitation in how DAP handles breakpoints that we can't do this inside DAP? What's the ramifications for other clients e.g. neovim?

[SeeminglyScience]

Hey @SeeminglyScience and @andyleejordan I can probably go thru and clean up conflicts to get this up to date

Ty for the offer but I just went through and cherry picked to a new branch (though that may not have been super necessary 🤷)

Is there a limitation in how DAP handles breakpoints that we can't do this inside DAP?

DAP is only active while debugging. Which makes perfect sense for most languages, but we're kinda always debugging (which would be annoying UX is we actually had an active debug session all the time)

What's the ramifications for other clients e.g. neovim?

The aim is no different than before this change.

[JustinGrote]

| What's the ramifications for other clients e.g. neovim? The aim is no different than before this change.

I guess more specifically, are we going to need to include implementation instructions for other language clients since this is not being handled exclusively in the DAP spec and using custom messages instead?

DAP is only active while debugging. Which makes perfect sense for most languages, but we're kinda always debugging (which would be annoying UX is we actually had an active debug session all the time)

This doesn't have to be the case as far as I can tell, DAP can be active and persist between debugging sessions. Rather than go through a full initialization and setup on every debug launch, we can do a single initialization, and keep breakpoints in sync between launch/attach reqeusts.

Per the spec under launch sequencing, we currently send all our breakpoint syncs after a launch, but we should instead be persistently keeping it up to date in between launches

Here's the view from my DAP trace of what we currently do:

When what we can do instead is:

1. Initialize
2. Set (and continue to set) breakpoints, syncing as they are changed in vscode and changed via set-breakpoint, etc.
3. When a launch happens, we could reverify just-prior (though if we do step 2 right we shouldn't have to)
4. launch/attaches continue to occur on the single persistent DAP session (might need different ones for each PSES temporary runspace/etc. but the same logic still applies)

If we do it this way then no special messaging has to be implemented in clients, the clients just have to maintain the DAP connection rather than shut it down between debug sessions, and the UI isn't affected (e.g. there isn't an ongoing "orange" debug because that only happens when we initiate a launch/attach task)

You've been working on this a lot longer than I have so maybe there's a limitation you discovered as to why we can't do it this way, I invite your feedback.

[SeeminglyScience]

Is there a specific part of the doc you linked that mentions multiple launches/terminates per initialize? Or are you inferring that from the name?

Unless it's changed, the behavior (and the way I'm reading that doc) was roughly "when a debug session is requested, send init, config, setbreakpoints/etc, launch, then wait for terminate". No messages in between debug sessions.

That's why we had to wait for some event we could hook into to even take a crack at rolling our own. Though again, maybe it's changed since then

[JustinGrote]

I would say it is somewhat inferred at the very end under "Debug Session End"

The end of a debug session is signified via a disconnect from the client or an exited from the debugee. Therefore the debugee is "terminated" (in our case we only fully exit the process if it was a launch to a separate terminal or remoting, etc.) and we signify that to the client that it has, but there's nothing that says the client can't initiate a new launch at that point. It also doesn't show the debug adapter terminating in addition to the debugee.

All language of terminate and disconnect refer to the debugee, NOT the debug adapter.

Based on my reading of this a debug session is defined as a superset, and multiple launch/attach/disconnect/terminate pairings can existing within the context of a debug session.

[JustinGrote]

Definition of debug session still appears to be vague https://github.com/microsoft/debug-adapter-protocol/issues/386 but vscode-js-debug reuses the adapter for multiple sessions in serial so it is de-facto supported I would say.

Multiplexing (simultaneous session in the same adapter) however is not supported, discussion here: https://github.com/microsoft/debug-adapter-protocol/issues/329, so we would need to maintain 1 debug adapter per debugee (PSIC, standalone terminal, remoting) or only support 1 PS debug at a time, so we'd probably need some sort of event hub to register for the breakpoint syncs so all can be updated if we went this route.

[JustinGrote]

Also, the setBreakpoint messages are still only client -> server, there's no way for the server to inform the client of a breakpoint change, only that a debug has been server-initiated with StartDebugging, so we would still need a custom S->C message in the case of Set-Breakpoint regardless.

Because we need custom messages anyways in that case then, I'd say there's no major need to change this, but maybe we can reduce the amount of custom implementation for clients in the future if some of the DAP protocol becomes more clear in this regard, like specifying a custom capability for server -> client breakpoint sync but using a persistent adapter to keep c->s breakpoints up to date per the spec.

Sorry for any potential thought derailment :)

[SeeminglyScience]

Sorry for any potential thought derailment :)

Oh yeah no worries. Rest assured I would have much rather just hooked up existing messages. Trying to roll your own is a huge PITA and more complicated than one would think. Skipping that would have been tops

[JustinGrote]

So a slight clarification, I misstated when I said there was no way for the server to inform the client of a breakpoint change, there is, the breakpoint event.

However as @SeeminglyScience stated, this only works "in" a debug session, and upon further reviewing the spec, I feel it's pretty clear that the end of a debug session means when the terminate/disconnect/exit happens, and another part states the debug adapter should terminate/disconnect, so this implies that there should only ever be one launch/attach request per debug session (aka adapter lifetime)

So it still stands that, unless we can have some sort of "background" attach session that doesn't surface in UIs and exchange messages that way, custom messages will be the way to go.

[JustinGrote]

So I had a crazy idea, what about using a custom configuration section to manage breakpoint state as a configuration?

There's nothing that says this has to be surfaced in vscode settings, and the type can be LSPAny which can include the breakpoint type. Client-Server is notified via Configuration Request, and Server-Client is notified via OnDidChangeConfiguration.

It would still require custom client configuration to hook the breakpoint set to the config, but it wouldn't require a custom message.