-
Notifications
You must be signed in to change notification settings - Fork 215
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
Sync breakpoints outside of debug sessions #1853
base: main
Are you sure you want to change the base?
Conversation
@@ -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. |
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!
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; | ||
} |
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
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!
@@ -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. |
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.
@@ -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) |
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
|
||
public void Unregister(SyncedBreakpoint breakpoint) | ||
{ | ||
ByGuid.TryRemove(breakpoint.Client.Id, out _); |
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 !
).
|
||
if (id is null) | ||
{ | ||
LogBreakpointError(newBreakpoint, "Did not receive a breakpoint ID from the client."); |
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.
VariableAccessMode mode = default; | ||
SyncedBreakpointKind kind = default; | ||
|
||
if (clientBreakpoint.FunctionName is not null) |
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.
script = clientBreakpoint.Location.Uri.Scheme is "untitled" | ||
? clientBreakpoint.Location.Uri.ToString() | ||
: clientBreakpoint.Location.Uri.GetFileSystemPath(); |
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).
{ | ||
// 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. |
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!
/// 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); |
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...
PowerShellExecutionOptions executionOptions, | ||
CancellationToken cancellationToken) | ||
=> InvokePSCommand<TResult>(psCommand, executionOptions, cancellationToken); | ||
|
||
public void InvokePSCommand(PSCommand psCommand, PowerShellExecutionOptions executionOptions, CancellationToken cancellationToken) => InvokePSCommand<PSObject>(psCommand, executionOptions, cancellationToken); |
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?
caa9e27
to
677f42c
Compare
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.