@TylerLeonhardt, your last commit had failures in PowerShell-CI-static-analysis

@PaulHigin I've made two more APIs public:

Debugger.GetBreakpoint(string id)
Debugger.GetBreakpoints()

It seemed like the right thing to do considering the SetBreakpoints method public.

This is now fully ready for review with tests.

@TylerLeonhardt, your last commit had 1 failures in PowerShell-CI-windows
Enable-RunspaceDebug -Breakpoint Unit Tests - Feature-Enabled.Error occurred in Describe block

The term 'New-PSBreakpoint' is not recognized as the name of a cmdlet, function, script file, or operable program.
Check the spelling of the name, or if a path was included, verify that the path is correct and try again.
At D:\a\1\s\test\powershell\Modules\Microsoft.PowerShell.Utility\Enable-RunspaceDebug.Tests.ps1:64 char:27

@PoshChan please restart CI-windows

@TylerLeonhardt, I do not understand: please restart CI-windows

@TylerLeonhardt, your last commit had 56 failures in PowerShell-CI-windows
(These are 5 of the failures)

ParserTests (admin\monad\tests\monad\src\engine\core\ParserTests.cs).Numerical Notations Tests (starting at line 2374 to line 2452).0b0 should return 0, with type [int]

Expected '0', but got $null.
at <ScriptBlock>, D:\a\1\s\test\powershell\Language\Parser\Parser.Tests.ps1: line 949
949:             ExecuteCommand $Script | Should -Be $ExpectedValue

ParserTests (admin\monad\tests\monad\src\engine\core\ParserTests.cs).Numerical Notations Tests (starting at line 2374 to line 2452).0b10 should return 2, with type [int]

Expected '2', but got $null.
at <ScriptBlock>, D:\a\1\s\test\powershell\Language\Parser\Parser.Tests.ps1: line 949
949:             ExecuteCommand $Script | Should -Be $ExpectedValue

ParserTests (admin\monad\tests\monad\src\engine\core\ParserTests.cs).Numerical Notations Tests (starting at line 2374 to line 2452).-0b10 should return -2, with type [int]

Expected '-2', but got $null.
at <ScriptBlock>, D:\a\1\s\test\powershell\Language\Parser\Parser.Tests.ps1: line 949
949:             ExecuteCommand $Script | Should -Be $ExpectedValue

ParserTests (admin\monad\tests\monad\src\engine\core\ParserTests.cs).Numerical Notations Tests (starting at line 2374 to line 2452).0b11111111 should return -1, with type [int]

Expected '-1', but got $null.
at <ScriptBlock>, D:\a\1\s\test\powershell\Language\Parser\Parser.Tests.ps1: line 949
949:             ExecuteCommand $Script | Should -Be $ExpectedValue

ParserTests (admin\monad\tests\monad\src\engine\core\ParserTests.cs).Numerical Notations Tests (starting at line 2374 to line 2452).0b1111111111111111 should return -1, with type [int]

Expected '-1', but got $null.
at <ScriptBlock>, D:\a\1\s\test\powershell\Language\Parser\Parser.Tests.ps1: line 949
949:             ExecuteCommand $Script | Should -Be $ExpectedValue

The single Codacy failure is not important here.

@PoshChan please retry windows

@TylerLeonhardt, successfully started retry of PowerShell-CI-Windows

I don't mind flushing out the POC and submitting a PR. Some questions first:

As far as PSES goes, if you set the breakpoint in the runspace via Set-PSBreakpoint, and then you invoke Debug-Runspace, it won't do a break all because there is a breakpoint set. If you don't set a breakpoint (i.e. there are no breakpoints in the runspace at all), then it will do a break all. Is that what you need? Or do you very specifically need to be able to prevent break all under all circumstances?

do you very specifically need to be able to prevent break all under all circumstances

Yes. Under no circumstances should a editor debugger break unless:

1. It hits a breakpoint
2. A "break immediately" setting is set to true (there's a feature request for this actually in PSES)

Got it, thanks.

@TylerLeonhardt: FYI, I'm making good progress on this, but I have something I need help with.

Enable-/Disable-/Remove-PSBreakpoint all have a -Breakpoint parameter that accepts pipeline input. If I retrieve a breakpoint from a specific runspace and pipe that into one of those commands, I need to know the runspace (really I need the ScriptDebugger, but I can get that from the runspace) they come from. But if I add the runspace, or the ScriptDebugger directly as I had originally thought I would do, it will break the rehydration that those objects support today. Instead I can work around this by storing the runspace instanceid in the breakpoint, since that would allow rehydration to continue to work, and from that guid I can get the runspace and then update the breakpoint accordingly when one is passed in via pipeline input.

If I go for job support as well, I could also store the job instance id on the breakpoint as well, so that I can pipe breakpoints that I'm managing in a job, and have the runspace instance id for use within that job to set the breakpoint accordingly.

Does that make sense? Any problems you forsee with this approach before I go down this path?

I'd like to hear @PaulHigin's thoughts on this.

Adding @PaulHigin's reply that happened on in email discussion about this.

I think it is a good idea to add a -Runspace parameter arguments to the breakpoint cmdlets. I believe this will meet Tyler’s needs, but we need to ensure all scenarios are covered. I don’t think it is necessary to update the Breakpoint type to include RunspaceId property. It think it is an unlikely scenario to pass breakpoint via pipeline over a remote session. For local use, passing via pipeline is likely and in this case we can simply append a note property to the Breakpoint type, that the updated cmdlets will recognize.

Changing the Breakpoint type creates remoting compatibility issues that I would like to avoid.

One thing you'll wanna watch out for is that the RemoteDebugger type does not have any SetBreakpoint() APIs.

Paul's recommendation is to add to/leverage this part of the code to handle setting the breakpoints on the remote runspace

Could we consolidate results of the discussion in new Issue to have publicly a clear plan for community?

Keep in mind that modifying RemoteDebugger function means we also have to update the remoting protocol version and document, and ensure backwards compatibility.

@PaulHigin Out of curiousity, why would you recommend modifying PreProcessDebuggerCommand instead of either...

1. ...adding Set*Breakpoint abstract methods to the abstract base Debugger class and then implementing virtual overrides on the derived debugger classes; or
2. ...adding Set*Breakpoint methods to the RemoteDebugger class that are not overrides?

What does using PreProcessDebuggerCommand buy us that Set*Breakpoint methods do not?

A RemoteDebugger is a pointer to a ScriptDebugger on the other end. The ScriptDebugger holds all the breakpoints used in a debug session. There’s currently no reference (in code) to a ScriptDebugger from within a RemoteDebugger.

The idea is that within the Set*Breakpoint methods in the RemoteDebugger class, you’d send a message using the remoting protocol to set the breakpoints within the ScriptDebugger on the other end.

Does that make sense?

@TylerLeonhardt Technically that makes sense to me, sure, and I have no problem implementing it that way, but I still wonder why you would suggest that approach vs making the Set*Breakpoint methods abstract on the abstract base Breakpoint class, so that those methods could directly invoke the corresponding methods on the ScriptDebugger on the other side, just as the GetBreakpoint/GetBreakpoints methods do here.

Yes, as Tyler mentions, the PreProcessDebuggerCommand is where we process remote debugging commands as defined in the protocol.

I appreciate the extra details @TylerLeonhardt and @PaulHigin, but I'm still missing some pieces of the puzzle. The fog is clearing up a little though. Here's where I'm at right now:

1. There is a remote debugging protocol that defines how PowerShell debugging should work remotely, and it needs to be updated (the version and the document), to ensure backwards compatibility.

   I had been looking for this document in the repository, but couldn't find it. Then I found it here.

   Is this in GitHub somewhere, so that updates can be added and the document versioned in a GitHub repository, and then the pdf/docx files get generated from markdown? Or is this something you gentlemen manage internally?

2. Is the reason you are proposing that we extend the protocol and add the new messages to allow breakpoints to be set in remote script debuggers is so that someone can then use that protocol definition to communicate with a remote script debugger and do the same, independent of the PowerShell runtime? If so, can you give me an example where this is done?

3. Today we have GetBreakpoint and GetBreakpoints public methods on a remote debugger that invoke corresponding GetBreakpoint and GetBreakpoints methods on the remote runspace debugger. Why don't these also work through messages sent to the remote script debugger via an extension to the remote debugging protocol? Or, the flipside: why not just expose abstract Set*Breakpoint methods on the Breakpoint base class, and then override them in other classes that derive from Debugger? i.e. Why do we want to get breakpoints via method invocation and set breakpoints via the remote debugging protocol?

If those questions don't make sense, please let me know.

The MS-PSRP and MS-PSRDP protocol documents are Windows documents that we manage internally.

The protocol is how we execute remote debugging commands over the wire, so we have to extend it when adding new functionality, and this includes updating the document.

This was not implemented originally because there was no need for it. But now that there is, we need to do it using the current protocol implementation mechanism. You can look at the 'PreProcessDebuggerCommand' in the 'ServerRunspacePoolDriver.cs' source file to see how other functions are implemented.

Thanks @PaulHigin.

On your last reply, I should add one point of clarification that is needed. When @TylerLeonhardt implemented this PR, one of the changes he made was to add GetBreakpoint and GetBreakpoints as abstract methods in the abstract base Breakpoint class. Since those recent additions were added that way, that is why I was asking where you draw the line on when to implement as methods that don't go over the wire via the MS-PSRDP, and when to go that route, adding to PreProcessDebuggerCommand.

@TylerLeonhardt, based on what I've heard from Paul here, it sounds like GetBreakpoint and GetBreakpoints should also be extensions to the MS-PSRDP and pull breakpoints from a remote debugger via PreProcessDebuggerCommand additions instead of using the changes you made in this PR that extended the base class with abstract methods. Please let me know if you agree, because if so, I'll go ahead and make those changes.

Yep, after further inspection of my code, I made an assumption that the RemoteDebugger (which has a Debugger property) was a reference to the ScriptDebugger... But it's not the case. It's totally circular (still a Remote Debugger type)

myRemoteDebugger.Debugger.Debugger.Debugger.Debugger

@TylerLeonhardt Thank you for confirming, and @PaulHigin, thank you for the protocol details. This all makes so much more sense now. :)

What is conclusion about the PR? Should we revert it?

I'd prefer just fixing it with another PR. I'm already well on my way with those changes, and many of the changes Tyler made will remain in place.

@PaulHigin Am I correct in my understanding (based on reading the code) that "Get" commands sent over the MS-PSRDP do not use the pre-processor, because they need to return data back to the caller and it doesn't seem that there is a way to do that aside from creating/invoking a PowerShell instance?

For example, the only "Get" command prior to needing to add GetBreakpoint(int id) and GetBreakpoints() is GetDebuggerStopArgs, which uses this enumeration: PreProcessCommandResult.GetDebuggerStopArgs. That enumeration value isn't used anywhere (i.e. it's functionally equivalent to PreProcessCommandResult.None), so when it is returned from PreProcessDebuggerCommand, it just bypasses the switch statement, so terminateImmediate remains set to false, resulting in the command running through the runspace.

Yes, that is correct. For any over the wire debugger command that doesn't return data and just sets debugger state, PreProcessDebuggerCommand() simply sets the debugger state and stops. But for for debugger commands that returns data (currently only GetDebuggerStopArgs), a script has to run so that data can be returned via the normal remoting mechanism.

Hey @PaulHigin, I was trying to get this PR submitted today, but stumbled upon an ugly bug in how breakpoints are handled that got in the way. I want to fix that bug as part of the PR I'm working on. It's a pretty weird bug, and definitely needs fixing.

Here's a repro scenario that works in 5.1 and 6.2.1 that should work in 7 preview as well:

# Create a job that writes a bunch of different types of stream data
$job = Start-Job -ScriptBlock {
    1..1000 | ForEach-Object {
        Start-Sleep -Seconds 1
        $_
        Write-Error 'boo'
        Write-Verbose 'Verbose' -Verbose
        $DebugPreference = 'Continue'
        Write-Debug 'Debug'
        Write-Warning 'Warning'
    }
}

# Set a local breakpoint on the Get-Process command
Set-PSBreakpoint -Command Get-Process

# Take note that the breakpoint with ID 0 is a command breakpoint for Get-Process

# Debug the job
Debug-Job -Job $job

# In your nested debugger, create a breakpoint inside of the job that triggers when the DebugPreference variable is written
Set-PSBreakpoint -Variable DebugPreference

# Detach your debugger
d

# Now that you're back in your main PowerShell runspace, have a look at your breakpoints
Get-PSBreakpoint

The reason this happens is because of an event loop. The ScriptDebugger.HandleBreakpointUpdated event handler is added to job debuggers and nested debuggers, and when you set or remove a breakpoint from inside a job debugger or a nested debugger, the event handler fires, internally invoking the corresponding set or remove breakpoint on the parent ScriptDebugger. This means breakpoints you set or remove in a child debugging session will result in updates to breakpoints with the same IDs in the parent debugger, even if those are not the same breakpoints at all. This is obviously incorrect, and I'm sure this has made someone scratch their head a few times. Enable and disable don't have the same effect, because they just enable or disable the breakpoint object that was already enabled or disabled, again.

What I could use some help on here is how best to solve it. What was the original intent behind having a job or nested debugger trigger an event handler in a parent ScriptDebugger instance? With the changes I'm making, users will be able to manage breakpoints in one debugger independently from another, so I tend to think that there is really no need for the ScriptDebugger.HandleBreakpointUpdated event handler at all, but I don't have the history behind what is there now to understand what was supposed to happen. Can you share some of the backstory behind the intent here so that I can confirm whether or not removing this event handler is the right thing to do?

Please give this a look and let me know how you feel I should proceed as soon as you can.

Thanks!

cc: @TylerLeonhardt

It feels to me that this work is getting out of hand. I think we need to take a step back and be more clear about what debugging changes need to be made, the scope and motivation of those changes. The best way to do this is via RFC and design documents.

Let's start with an RFC and a short summary of what we want to achieve along with scenarios we want to support. Remote and job debugging support may be a separate effort.

cc: SteveL-MSFT, @TylerLeonhardt

@PaulHigin What?!?

That is a horrible suggestion. Seriously, just horrible.

I'm just a contributor here, but I've put a lot of time into this. I've pushed another PR on the side because I felt getting this corrected (the design of this functionality and the issues in its implementation) were more important than finishing up tests on the other PR work.

Now you want to put all of that work on hold when I'm just about to submit a PR, slam on the brakes, and push it through the terribly slow RFC process, where I have many, many RFCs that just are not progressing, throwing my efforts to get some of these things corrected and into PowerShell 7 out the door completely? Thanks but no thanks.

The original work @TylerLeonhardt did skipped the RFC process, and was merged. Now here we are with me having finished my dev work, but I found a bug that should be looked at, so I reached out because I can't make assumptions about why the existing implementation is there, and I don't have the luxury of being on your team to talk directly to you about this, so I ask questions here. I know that interrupts your other work. I'm trying to help you guys, and PowerShell as a whole (the product and the community). You're making it really hard for me to do that.

cc: @SteveL-MSFT

@KirkMunro

I understand your frustration, but we need a way to manage extensive changes to the code base and RFCs are the current way to do that. PRs are fine for small scope changes, but I feel this has gone beyond that and we need step back and think carefully about what we want to do and how to move forward.

I know that RFCs are a slow process, but really it is meant to be so that we don't make decisions we later regret.

Also, I have other high priority work to do so I cannot always be as responsive as I would like.

Regarding RFCs, I find that smaller, well scoped, clear and concise writing gets better and quicker responses. Verbose writing is harder to parse and understand. Large scope changes are harder approve since it is difficult to think through all implications, including compatibility and documentation. In general I prefer incremental changes where possible.

Just to add… the original scope of this work was to make the VSCode extension better by providing APIs (as in PowerShell’s C# API) for managing breakpoints.

We expanded on that by exposing a few additions to cmdlets so that folks debugging from the CLI could take advantage of these features - thus New-PSBreakpoint and the -Breakpoint param were born.

Upon seeing @KirkMunro’s original thoughts on cmdlet design, I agreed that New-PSBreakpoint was probably less “PowerShelly” compared to Set-PSBreakpoint -Runspace and @PaulHigin and I both felt like that’s probably the better way to go and @KirkMunro took that and ran with it.

As @KirkMunro continued this work, it’s clear that there are edge cases in the debugging stack. That’s not surprising - it’s a piece of software.

I personally think that we should not include any bug fixes with this change and instead, should address bug fixes in a separate PR so to make the size of the PR that adds this feature smaller.

The PR should only:

• give the vscode extension an API to manage breakpoints in other runspaces
• give a simple cmdlet experience so that users of the CLI can do similar breakpoint management in other runspaces.

give the vscode extension an API to manage breakpoints in other runspaces

This is why we didn’t originally open an RFC for this is because the extension needs this functionality to act in the way other language debuggers in vscode do. It was scoped to this.

I'm good with that, for the most part. The other issues will quickly fall into my short list of items to resolve after this work.

Even if you disagree strongly with something, please remember to follow our code of conduct when posting in issues or PRs. Locking out the rest of this one as I think we're closed here.

If you still want to push for some of your changes that weren't merged, feel free to open a discussion issue.

I'll be publishing a blog on RFCs and feature additions soon, hopefully it should address some of the confusion around scenarios like this one.