Sync breakpoints outside of debug sessions #1853

SeeminglyScience · 2022-07-08T19:50:40Z

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.

SeeminglyScience · 2022-07-08T19:53:44Z

src/PowerShellEditorServices/Index.cs

@@ -0,0 +1,154 @@
+// <auto-generated>
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.


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.

I think we chatted about this and it was fine.

andyleejordan

Ok I got through some of this!

src/PowerShellEditorServices/Server/PsesLanguageServer.cs

andyleejordan · 2022-07-12T23:26:59Z

src/PowerShellEditorServices/Services/DebugAdapter/BreakpointSyncService.cs

+        if (!_map.ByPwshId.TryGetValue(eventArgs.Breakpoint.Id, out SyncedBreakpoint existing))
+        {
+            // If we haven't told the client about the breakpoint yet then we can just ignore.
+            if (eventArgs.UpdateType is BreakpointUpdateType.Removed)
+            {
+                return;
+            }
+
+            existing = CreateFromServerBreakpoint(eventArgs.Breakpoint);
+            string? id = await SendToClientAsync(existing.Client, BreakpointUpdateType.Set)
+                .ConfigureAwait(false);
+
+            existing.Client.Id = id!;
+            RegisterBreakpoint(existing);
+            return;
+        }


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?

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

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

src/PowerShellEditorServices/Services/DebugAdapter/Handlers/LaunchAndAttachHandler.cs

src/PowerShellEditorServices/Services/PowerShell/Host/PsesInternalHost.cs

The assert here didn't really make sense, since when it's serialized to json `Id` will obviously be accessed.

andyleejordan

Reviewed! Thinking we should do a team review too. But this is very exciting, let's get it into the next preview!

andyleejordan · 2023-10-16T23:23:02Z

src/PowerShellEditorServices/Index.cs

@@ -0,0 +1,154 @@
+// <auto-generated>
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.


I think we chatted about this and it was fine.

andyleejordan · 2023-10-16T23:26:06Z

src/PowerShellEditorServices/Services/DebugAdapter/BreakpointService.cs

@@ -227,6 +247,13 @@ public async Task<IReadOnlyList<CommandBreakpointDetails>> SetCommandBreakpoints
        /// </summary>
        public async Task RemoveAllBreakpointsAsync(string scriptPath = null)
        {
+            // Only need to remove all breakpoints if we're not able to sync outside of debug
+            // sessions.
+            if (_breakpointSyncService?.IsSupported is true)


Should this check actually be wherever we call RemoveAllBreakpointsAsync?

src/PowerShellEditorServices/Services/DebugAdapter/BreakpointSyncService.cs

andyleejordan · 2023-10-16T23:27:47Z

src/PowerShellEditorServices/Services/DebugAdapter/BreakpointSyncService.cs

+
+        public void Unregister(SyncedBreakpoint breakpoint)
+        {
+            ByGuid.TryRemove(breakpoint.Client.Id, out _);


In fact the C# analyzer on GitHub is complaining about Client.Id being possibly null here (since it isn't ignored via !).

andyleejordan · 2023-10-16T23:36:28Z

src/PowerShellEditorServices/Services/DebugAdapter/BreakpointSyncService.cs

+
+        if (id is null)
+        {
+            LogBreakpointError(newBreakpoint, "Did not receive a breakpoint ID from the client.");


Can we proceed without a client ID?

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.

andyleejordan · 2023-10-16T23:58:19Z

src/PowerShellEditorServices/Services/DebugAdapter/BreakpointSyncService.cs

+        VariableAccessMode mode = default;
+        SyncedBreakpointKind kind = default;
+
+        if (clientBreakpoint.FunctionName is not null)


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.

andyleejordan · 2023-10-16T23:58:51Z

src/PowerShellEditorServices/Services/DebugAdapter/BreakpointSyncService.cs

+            script = clientBreakpoint.Location.Uri.Scheme is "untitled"
+                ? clientBreakpoint.Location.Uri.ToString()
+                : clientBreakpoint.Location.Uri.GetFileSystemPath();


Another thing to double-check (since I think we have a "is URI untitled" helper function with unit tests).

andyleejordan · 2023-10-17T00:03:25Z

src/PowerShellEditorServices/Services/DebugAdapter/Handlers/BreakpointHandlers.cs

+            {
+                // TODO: Find some way to actually verify these. Unfortunately the sync event comes
+                // through *after* the `SetBreakpoints` event so we can't just look at what synced
+                // breakpoints were successfully set.


andyleejordan · 2023-10-17T00:04:46Z

src/PowerShellEditorServices/Services/PowerShell/IPowerShellExecutionService.cs

+        /// the work for the pipeline thread. This method must only be invoked when the caller
+        /// has ensured that they are already running on the pipeline thread.
+        /// </summary>
+        IReadOnlyList<TResult> UnsafeInvokePSCommand<TResult>(PSCommand psCommand, PowerShellExecutionOptions executionOptions, CancellationToken cancellationToken);


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...

andyleejordan · 2023-10-17T00:05:43Z

src/PowerShellEditorServices/Services/PowerShell/Host/PsesInternalHost.cs

+            PowerShellExecutionOptions executionOptions,
+            CancellationToken cancellationToken)
+            => InvokePSCommand<TResult>(psCommand, executionOptions, cancellationToken);
+
        public void InvokePSCommand(PSCommand psCommand, PowerShellExecutionOptions executionOptions, CancellationToken cancellationToken) => InvokePSCommand<PSObject>(psCommand, executionOptions, cancellationToken);


Perhaps we should make this private and require the Unsafe versions be called?

JustinGrote · 2024-10-11T02:02:36Z

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 · 2024-10-11T16:12:06Z

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 · 2024-10-11T16:46:11Z

| 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:

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.

SeeminglyScience · 2024-10-11T17:20:31Z

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 · 2024-10-11T17:55:46Z

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 · 2024-10-11T18:09:35Z

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.

JustinGrote · 2024-10-11T18:21:45Z

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 · 2024-10-11T18:49:12Z

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 · 2024-10-16T23:19:52Z

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 · 2024-10-19T15:03:21Z

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.

SeeminglyScience added 4 commits July 8, 2022 14:47

Add NRT attribs polyfill

17e0d3f

Add index and range polyfills

ec8806f

Add new services for syncing bps outside of DAP

5e7a614

hook up bp sync services

ef47274

SeeminglyScience mentioned this pull request Jul 8, 2022

Sync breakpoints outside of DAP with onDidChangeBreakpoints event PowerShell/vscode-powershell#4065

Draft

4 tasks

SeeminglyScience commented Jul 8, 2022

View reviewed changes

andyleejordan reviewed Jul 12, 2022

View reviewed changes

andyleejordan force-pushed the main branch from cadf294 to 712c0f8 Compare July 13, 2023 01:30

SeeminglyScience and others added 7 commits September 13, 2023 13:00

Merge branch 'main' into bp-sync

f357fa1

Fix build errors

bed8cc3

Fix synced breakpoints for untitled files

347f500

Fix tests

17f3e8c

Fix crash in debug config

55c749d

The assert here didn't really make sense, since when it's serialized to json `Id` will obviously be accessed.

Add a note about alterations and ifdef to polyfill

d6971a9

Address PR feedback

8c10d8a

SeeminglyScience marked this pull request as ready for review October 16, 2023 16:11

SeeminglyScience requested a review from a team October 16, 2023 16:11

SeeminglyScience changed the title ~~WIP: Sync breakpoints outside of debug sessions~~ Sync breakpoints outside of debug sessions Oct 16, 2023

andyleejordan reviewed Oct 17, 2023

View reviewed changes

andyleejordan force-pushed the main branch from e390755 to db3d833 Compare March 27, 2024 19:05

andyleejordan force-pushed the main branch 2 times, most recently from caa9e27 to 677f42c Compare May 3, 2024 23:07

JustinGrote mentioned this pull request Oct 8, 2024

Don't stop debugging after script execution in TemporaryIntegratedConsole #2180

Open

2 tasks

Merge branch 'main' into bp-sync

6019568

andyleejordan force-pushed the main branch 2 times, most recently from 80b2351 to 9ce8911 Compare November 18, 2024 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync breakpoints outside of debug sessions #1853

Sync breakpoints outside of debug sessions #1853

SeeminglyScience commented Jul 8, 2022 •

edited

Loading

SeeminglyScience Jul 8, 2022

andyleejordan Oct 16, 2023

andyleejordan left a comment

andyleejordan Jul 12, 2022

SeeminglyScience Jul 13, 2022

SeeminglyScience Sep 20, 2023

andyleejordan left a comment

andyleejordan Oct 16, 2023

andyleejordan Oct 16, 2023

andyleejordan Oct 16, 2023

andyleejordan Oct 16, 2023

andyleejordan Oct 16, 2023

andyleejordan Oct 16, 2023

andyleejordan Oct 16, 2023

andyleejordan Oct 17, 2023

andyleejordan Oct 17, 2023

andyleejordan Oct 17, 2023

JustinGrote commented Oct 11, 2024

SeeminglyScience commented Oct 11, 2024

JustinGrote commented Oct 11, 2024 •

edited

Loading

SeeminglyScience commented Oct 11, 2024

JustinGrote commented Oct 11, 2024

JustinGrote commented Oct 11, 2024

JustinGrote commented Oct 11, 2024 •

edited

Loading

SeeminglyScience commented Oct 11, 2024

JustinGrote commented Oct 16, 2024

JustinGrote commented Oct 19, 2024 •

edited

Loading

Sync breakpoints outside of debug sessions #1853

Are you sure you want to change the base?

Sync breakpoints outside of debug sessions #1853

Conversation

SeeminglyScience commented Jul 8, 2022 • edited Loading

PR Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyleejordan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyleejordan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JustinGrote commented Oct 11, 2024

SeeminglyScience commented Oct 11, 2024

JustinGrote commented Oct 11, 2024 • edited Loading

SeeminglyScience commented Oct 11, 2024

JustinGrote commented Oct 11, 2024

JustinGrote commented Oct 11, 2024

JustinGrote commented Oct 11, 2024 • edited Loading

SeeminglyScience commented Oct 11, 2024

JustinGrote commented Oct 16, 2024

JustinGrote commented Oct 19, 2024 • edited Loading

SeeminglyScience commented Jul 8, 2022 •

edited

Loading

JustinGrote commented Oct 11, 2024 •

edited

Loading

JustinGrote commented Oct 11, 2024 •

edited

Loading

JustinGrote commented Oct 19, 2024 •

edited

Loading