-
Notifications
You must be signed in to change notification settings - Fork 238
Sync breakpoints outside of debug sessions #1853
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't 100% know what to do with this and Index
. I may have to rip these out and stop using the range syntax for arbitrary legal reasons but I'd like to check out what other MS projects are doing to polyfill.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we chatted about this and it was fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I got through some of this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a little confusing to have this in an if (!getThing(named out param))
as the out param is not used (since it's not, it wasn't in the map) but we then assign to it locally and use that. I'd do out Thing _
and then locally declare Thing breakpoint
. Or did I read this wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's used outside of that if
block. The if block is the exit early condition for it not already existing.
I can for sure see how that's confusing though I'll think of a better way to write this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright I rewrote this to call a nested method and added a new variable instead of reusing existing
since that was really confusing. LMK what you think!
src/PowerShellEditorServices/Services/DebugAdapter/BreakpointSyncService.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/DebugAdapter/BreakpointSyncService.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/DebugAdapter/BreakpointSyncService.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/DebugAdapter/BreakpointSyncService.cs
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/DebugAdapter/Handlers/LaunchAndAttachHandler.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/DebugAdapter/Handlers/LaunchAndAttachHandler.cs
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/PowerShell/Host/PsesInternalHost.cs
Show resolved
Hide resolved
cadf294
to
712c0f8
Compare
The assert here didn't really make sense, since when it's serialized to json `Id` will obviously be accessed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed! Thinking we should do a team review too. But this is very exciting, let's get it into the next preview!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we chatted about this and it was fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this check actually be wherever we call RemoveAllBreakpointsAsync
?
src/PowerShellEditorServices/Services/DebugAdapter/BreakpointSyncService.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact the C# analyzer on GitHub is complaining about Client.Id
being possibly null here (since it isn't ignored via !
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we proceed without a client ID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the ID shouldn't be nullable if we don't get one, we skip it. Seems like everything else expects/requires it be defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This clause could use commentary...I think it's first checking if we were given a variable name and if so resolving it to the function name and otherwise assuming we were given the function name directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing to double-check (since I think we have a "is URI untitled" helper function with unit tests).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Annoying!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should actually use this to process user input from the prompt when we're already on the thread receiving said input...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should make this private and require the Unsafe
versions be called?
e390755
to
db3d833
Compare
caa9e27
to
677f42c
Compare
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?
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.
| 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
image
Here's the view from my DAP trace of what we currently do:
image
When what we can do instead is:
- Initialize
- Set (and continue to set) breakpoints, syncing as they are changed in vscode and changed via set-breakpoint, etc.
- When a launch happens, we could reverify just-prior (though if we do step 2 right we shouldn't have to)
- 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.
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
I would say it is somewhat inferred at the very end under "Debug Session End"
image
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.
Definition of debug session still appears to be vague microsoft/debug-adapter-protocol#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: microsoft/debug-adapter-protocol#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.
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 :)
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
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.
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.
80b2351
to
9ce8911
Compare
Uh oh!
There was an error while loading. Please reload this page.
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.