E52B WIP: Breakpoint management for runspaces by KirkMunro · Pull Request #10189 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

WIP: Breakpoint management for runspaces #10189

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.

Sign up for GitHub

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

Jump to bottom
Closed
KirkMunro wants to merge 14 commits into from
Closed

WIP: Breakpoint management for runspaces #10189

KirkMunro wants to merge 14 commits into from

Conversation

@KirkMunro
Copy link
Contributor
@KirkMunro KirkMunro commented Jul 19, 2019 •
edited
Loading

PR Summary

  • Adds an experimental flag called "Microsoft.PowerShell.Utility.PSManageBreakpointsInRunspace". All experimental functionality described below is enabled/disabled using that flag.
  • Adds an experimental -Runspace parameter Get-PSBreakpoint, Set-PSBreakpoint, Enable-PSBreakpoint, Disable-PSBreakpoint, and Remove-PSBreakpoint.
  • Refactors the classes behind the *-PSBreakpoint cmdlets so that they all work consistently. This refactoring did not change the behavior of these cmdlets beyond adding support for runspace breakpoint management.
  • Removes the recently added New-PSBreakpoint experimental cmdlet in favor of working with the older, more well known *-PSBreakpoint cmdlets for runspace breakpoint management.
  • Refactors DebugRunspaceCommand and DebugJobCommand so that they both derive from a common PSRemoteDebugCmdlet base class. As part of this refactoring, some of the resource strings have been moved to be co-located with the new base class where they are used.
  • Fixes a very minor issue where a remote breakpoint would not be displayed properly in the host when entering a remote debugger.
  • Adds SetCommandBreakpoint, SetVariableBreakpoint, SetLineBreakpoint, EnableBreakpoint, DisableBreakpoint, and RemoveBreakpoint public APIs to the Debugger class, plumbs them through the engine, and hooks them up for remote debugger work. Also hooks up the GetBreakpoint, GetBreakpoints, and SetBreakpoints Debugger class APIs for remote debugger work.
  • Adds a -NoInitialBreak experimental parameter to Debug-Runspace and Debug-Job that allows users to attach the debugger without invoking a BreakAll. The corresponding methods for the same have boolean parameters allowing this as well.

PR Context

This PR was opened in response to this discussion between @TylerLeonhardt, @PaulHigin and I.

It helps because:

  • It allows scripters to fully manage breakpoints in a remote runspace without having to use Debug-* to attach the debugger to that runspace. This provides a better, more PowerShell-oriented approach to debugging.
  • It allows @TylerLeonhardt to attach a debugger to a runspace without invoking a BreakAll in PSES.
  • It simplifies the implementation behind *-PSBreakpoint cmdlets.
  • The implementation behind Debug-Runspace and Debug-Job, as defined in DebugRunspaceCommand and DebugJobCommand, respectively, had a lot of duplicated logic and a few unintentional differences. Moving these classes into a shared base class structure eliminates the unintentional differences and removes the duplicated code.

PR Checklist

@KirkMunro KirkMunro mentioned this pull request Jul 19, 2019
Closed
12 tasks
@KirkMunro KirkMunro changed the title Breakpoint management for runspaces WIP: Breakpoint management for runspaces Jul 20, 2019
@KirkMunro KirkMunro changed the title WIP: Breakpoint management for runspaces Breakpoint management for runspaces Jul 21, 2019
@iSazonov iSazonov added this to the 7.0.0-preview.3 milestone Jul 22, 2019
iSazonov
iSazonov reviewed Jul 22, 2019
View reviewed changes
@iSazonov
Copy link
Collaborator
iSazonov commented Jul 22, 2019

@KirkMunro Is it possible to split the PR on some small ones? The PR is so large that we can wait too long for approval.

@KirkMunro
Copy link
Contributor Author
KirkMunro commented Jul 22, 2019 •
edited
Loading

@KirkMunro Is it possible to split the PR on some small ones? The PR is so large that we can wait too long for approval.

That sounds like quite a bit more work, with little tangible value; however, a "review map" might help. Here is a break down of the changes in each file, with like changes grouped together to allow them to be reviewed more easily. Between these extra details and being able to mark files as viewed in the GitHub UI, I think it should be much easier for others to review this PR.

Cleanup of changes done in @TylerLeonhardt's other PR that this supercedes

src/Microsoft.PowerShell.Commands.Utility/commands/utility/New-PSBreakpoint.cs
src/Microsoft.PowerShell.Commands.Utility/commands/utility/EnableDisableRunspaceDebugCommand.cs

src/Microsoft.PowerShell.Commands.Utility/commands/utility/PSBreakpointCreationBase.cs

  • simple removal of previous refactoring work that was done in the same PR

src/Modules/Unix/Microsoft.PowerShell.Utility/Microsoft.PowerShell.Utility.psd1
src/Modules/Windows/Microsoft.PowerShell.Utility/Microsoft.PowerShell.Utility.psd1

src/System.Management.Automation/engine/InitialSessionState.cs

  • removed New-PSBreakpoint reference

test/powershell/engine/Basic/DefaultCommands.Tests.ps1

  • removed New-PSBreakpoint reference

test/powershell/Modules/Microsoft.PowerShell.Utility/Enable-RunspaceDebug.Tests.ps1

  • removed this file as it was based on the other PR that this supercedes

Changes for Pester tests

test/powershell/Modules/Microsoft.PowerShell.Utility/RunspaceBreakpointManagement.Tests.ps1

  • added tests for runspace breakpoint management (both local and remote runspaces)

test/tools/TestMetadata.json

  • updated to match new experimental feature name and test file

Changes to support -Runspace parameter in *-PSBreakpoint commands

src/Microsoft.PowerShell.Commands.Utility/commands/utility/RunspaceParameter.cs

  • defined a transformation attribute that converts integers, GUIDs and strings into
    runspaces; this is for use in the -Runspace parameter added to the *-PSBreakpoint
    cmdlets updated below (it keeps the number of parameter sets low in those commands
    while still supporting multiple ways to reference a runspace)

src/Microsoft.PowerShell.Commands.Utility/commands/utility/PSBreakpointCommandBase.cs
src/Microsoft.PowerShell.Commands.Utility/commands/utility/PSBreakpointAccessorCommandBase.cs
src/Microsoft.PowerShell.Commands.Utility/commands/utility/Get-PSBreakpoint.cs
src/Microsoft.PowerShell.Commands.Utility/commands/utility/Set-PSBreakpoint.cs
src/Microsoft.PowerShell.Commands.Utility/commands/utility/PSBreakpointUpdaterCommandBase.cs
src/Microsoft.PowerShell.Commands.Utility/commands/utility/Disable-PSBreakpoint.cs
src/Microsoft.PowerShell.Commands.Utility/commands/utility/Enable-PSBreakpoint.cs
src/Microsoft.PowerShell.Commands.Utility/commands/utility/Remove-PSBreakpoint.cs

  • added -Runspace parameter to each *-PSBreakpoint cmdlet
  • standardized parameter set names across this set of cmdlets
  • refactored *-PSBreakpoint cmdlets into class hierarchy, as follows: 
                           PSBreakpointCommandBase
                          /              \
PSBreakpointAccessorCommandBase        PSBreakpointUpdaterCommandBase
             |                                       |
 Get/SetPSBreakpointCommand        Disable/Enable/RemovePSBreakpointCommand

Changes to support managing breakpoints in debugger via the SDK

src/System.Management.Automation/engine/debugger/Breakpoint.cs

  • fixed minor issue in display of breakpoint that was hit when attaching
    the debugger to a remote runspace
  • updated RemoveSelf method to return a bool if a breakpoint was removed

src/System.Management.Automation/engine/debugger/debugger.cs

  • added SetCommandBreakpoint, SetLineBreakpoint, SetVariableBreakpoint,
    RemoveBreakpoint, EnableBreakpoint, and DisableBreakpoint public methods
    in Debugger class
  • added DebugJob method that allows you to attach a debugger to a job
    without invoking a BreakAll
  • changed return type of AddCommandBreakpoint, AddLineBreakpoint and
    AddVariableBreakpoint on ScriptDebugger class to be specific to the type
    of breakpoint they add
  • removed NewBreakpoint methods on ScriptDebugger class (these have been
    superceded by the Set    Breakpoint public methods)
  • updated Remove*Breakpoint methods to return a bool indicating whether or
    not a breakpoint was removed
  • defined overrides in ScriptDebugger for the *Breakpoint methods that were
    added to the base class, and grouped them together (this involved replacing
    some methods that were previously internal)
  • moved a few methods into proper groupings according to existing regions
  • defined overrides in NestedDebugger for the *Breakpoint methods that were
    added to the base class
  • added internal virtual function names for breakpoint functions that can now
    be invoked in a remote runspace

src/System.Management.Automation/engine/remoting/client/remoterunspace.cs

  • added overrides to remote debugger for the *Breakpoint methods that were
    added to the base class
  • added internal helper method to handle invoking those methods remotely,
    capturing their results, and processing any errors

src/System.Management.Automation/engine/remoting/client/Job.cs

  • added IsFinished property to simplify code elsewhere that checks if a job
    is finished
  • added new Breakpoint methods to JobDebugger class and grouped them together

src/System.Management.Automation/engine/server/ServerRunspacePoolDriver.cs

  • added support for the new *Breakpoint virtual functions to the
    PreProcessDebuggerCommand method
  • added overrides for *Breakpoint methods to ServerRemoteDebugger class
  • added override for new DebugJob overload method to ServerRemoteDebugger class

src/System.Management.Automation/engine/serialization.cs

  • added RehydrationFlags.NullValueOk to Script and Action properties of
    breakpoints (this ensures they are set to null instead of empty when rehydrated,
    which matches how they are set before they are dehydrated)

src/System.Management.Automation/engine/hostifaces/PSDataCollection.cs

  • added AddRange method to simplify adding a collection to a collection

Addition of -NoInitialBreak parameter to Debug-Job and Debug-Runspace, plus refactoring

src/Microsoft.PowerShell.Commands.Utility/resources/Debugger.resx
src/System.Management.Automation/resources/RemotingErrorIdStrings.resx
src/System.Management.Automation/resources/DebuggerStrings.resx

  • moved a bunch of resource strings into src/System.Management.Automation/resources/DebuggerStrings.resx
    as part of the refactoring of DebugRunspaceCommand and DebugJobCommand under
    a common base class
  • added some resource strings

src/System.Management.Automation/engine/remoting/commands/PSRemoteDebugCmdlet.cs
src/Microsoft.PowerShell.Commands.Utility/commands/utility/DebugRunspaceCommand.cs
src/System.Management.Automation/engine/remoting/commands/DebugJob.cs

  • added -NoInitialBreak parameter to Debug-Runspace and Debug-Job cmdlets
  • moved common code out of the cmdlet classes into new PSRemoteDebugCmdlet class
  • refactored these two cmdlets to eliminate unintended differences in the UX they
    provided into a class hierarchy, as follows: 
               PSRemoteDebugCmdlet
             /              \
DebugJobCommand        DebugRunspaceCommand

@PaulHigin
Copy link
Contributor
PaulHigin commented Jul 22, 2019

I'll try to take a look at this later this week, but may not get to it until the following week. Since this is such a large change I will mark it for committee review.

@PaulHigin PaulHigin added Committee-Reviewed PS-Committee has reviewed this and made a decision Review - Committee The PR/Issue needs a review from the PowerShell Committee and removed Committee-Reviewed PS-Committee has reviewed this and made a decision labels Jul 22, 2019
@KirkMunro
Copy link
Contributor Author
KirkMunro commented Jul 23, 2019 •
edited
Loading

@PaulHigin FYI for when you do get to look at this:

I'm experiencing an intermittent issue with my RunspaceBreakpointManagement.Tests.ps1 Pester test series where setting a breakpoint in a remote runspace (i.e. one associated with a background job) gets stuck. On my system, when I don't have my VS 2019 debugger connected to the PowerShell session process, I can very often reproduce my intermittent issue the second time that I manually invoke that Pester test file in a single PowerShell session. All tests pass in the first run, but the second run hangs at a certain point (the moment the first MS-PSRDP call is made. Digging in to this on my system, I believe it's getting stuck between the moment it invokes the virtual __Set-PSBreakpoint function in a nested PowerShell instance, which should set the breakpoint on the remote runspace. The local process seems to be waiting for the pipeline to finish, and the debugger in the remote (job) process seems to be waiting for a client to connect to it, so this seems to have to do with the MS-PSRDP. If you wait long enough (30 minutes), the Pester test eventually fails (probably due to a timeout) with a PSRemotingTransportException.

In the PR that this supercedes, @TylerLeonhardt switched many of the collections to their concurrent equivalent, and he updated the breakpoint ID increment to be thread safe as well. Those changes were preserved through this PR work.

I know little about how the MS-PSRDP plumbing works other than what I learned while working on this PR, so I wanted to let you know what I ran into so that you could have a look when you get to this review work because so far I can't figure out the root cause.

@KirkMunro
Copy link
Contributor Author
KirkMunro commented Sep 6, 2019

Phase 2 of splitting this up: PR #10492, that includes the *-PSBreakpoint cmdlet changes.

@KirkMunro KirkMunro mentioned this pull request Sep 6, 2019
Merged
14 tasks
@ghost ghost assigned anmenaga Nov 23, 2019
@ghost ghost added the Review - Needed The PR is being reviewed label May 27, 2020
@ghost
Copy link
ghost commented May 27, 2020

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Mainainer, Please provide feedback and/or mark it as Waiting on Author

@iSazonov
Copy link
Collaborator
iSazonov commented May 27, 2020

@KirkMunro Please resolve merge conflicts.

@TravisEz13 TravisEz13 added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Needed The PR is being reviewed Review - Waiting on Author labels May 27, 2020
@ghost ghost added the Stale label Jun 11, 2020
@ghost
Copy link
ghost commented Jun 11, 2020

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment.

@ghost ghost removed this from the 7.1.0-preview.4 milestone Jun 11, 2020
@ghost ghost closed this Jun 22, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov left review comments

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

@anmenaga anmenaga Awaiting requested review from anmenaga

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw daxian-dbw is a code owner

@JamesWTruher JamesWTruher Awaiting requested review from JamesWTruher

@PaulHigin PaulHigin Awaiting requested review from PaulHigin

@TravisEz13 TravisEz13 Awaiting requested review from TravisEz13 TravisEz13 is a code owner

Assignees

@anmenaga anmenaga

Labels

Committee-Reviewed PS-Committee has reviewed this and made a decision Stale Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@KirkMunro @iSazonov @PaulHigin @SteveL-MSFT @daxian-dbw @TravisEz13 @anmenaga
0