Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

@KirkMunro
Copy link
Contributor

PR Summary

  • Adds an experimental flag called "Microsoft.PowerShell.Utility.PSManageBreakpointsInRunspace". All experimental functionality described below is enabled/disabled using that flag.
  • Removes the recently added New-PSBreakpoint experimental cmdlet. This functionality is provided via SDK commands for now, and will be provided via a -Runspace parameter on *-PSBreakpoint commands in a PR once this one is finalized.
  • 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.
  • Hooks up Set-PSBreakpoint to the new debugger APIs that have superceded the ones it used to use.

PR Context

This PR is needed for "attach to process" debugging in PowerShell Editor Services.

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.
  • It allows Tyler to attach a debugger to a runspace without invoking a BreakAll in PSES.

Important details about this PR:

  1. It does not include Pester tests for the -NoInitialBreak parameter added to Debug-Runspace and Debug-Job, nor does it include Pester tests for the corresponding boolean value in the DebugRunspace and DebugJob APIs. I'm not sure how to automate that test, and believe it should be tested manually. Fortunately, that is an easy manual test to take on.
  2. The tests in the new Breakpoint.Tests.ps1 file sometimes hang. I don't know why this happens. It has to do with working with a remote runspace and using the breakpoint APIs against it. It doesn't happen all the time, but it happens often. After 30 minutes, it will time out and fail. I need help identifying why this happens, by someone who understands the MS-PSRDP better than I do.

cc: @TylerLeonhardt @PaulHigin @SteveL-MSFT

PR Checklist

@KirkMunro
Copy link
Contributor Author

Looks like there are some Pester test failures that have nothing to do with this PR.

@KirkMunro
Copy link
Contributor Author

@PaulHigin: Regarding the intermittent hang, my suspicion is that there is a timing issue with the initial handshake in MS-PSRDP, where the initial outgoing message from the client fires before the server starts actively listening for requests, at which point both client and server are waiting for communication from one another: the client waits for the server to respond, and the server waits for the client to tell it what to do. I believe this is an existing issue, not new, and it can probably be reproduced with the original set of APIs that MS-PSRDP supports.

@KirkMunro
Copy link
Contributor Author

@PaulHigin Thinking about this more, and reading through the code this morning, I'm convinced that the scenario I shared above is the root cause, and the issue I tripped on is related to issue #8012.

Neither RunspaceState nor RunspacePoolState have a "Ready" state. They only have Opened, and they both of them do processing after they have entered the Opened state, which is the wrong thing to do when it comes to asynchronous processing. The test that hangs intermittently waits for the job to enter the Running state before using the new MS-PSRDP APIs, but jobs can enter a Running state before the listener is ready to receive requests in the remote runspace. When this happens, remote APIs invoked against the remote runspace will appear hung.

This issue is not related to the changes in this PR, but this PR and the original test suite written to validate these changes exposed the issue. That should be handled separately, as an RFC according to the committee review of #8012, which should propose a solution for both Runspace and RunspacePool event handling to ensure that SDK users can build code that can know with confidence that the runspace they are accessing via the SDK is ready for them to use it asynchronously. I've added that RFC to my list of tasks to do, so that we can get that discussion started and resolve this problem for SDK users who do async processing with PowerShell.

In the meantime, I just changed the Pester tests that use the new MS-PSRDP APIs such that they won't run until the runspace is truly ready. The easiest workaround I could come up with was to add a breakpoint and wait for the runspace to hit that breakpoint before using the APIs. Making this change allows me to run the tests over and over and over, and it never hangs. I'll commit that change in just a moment.

@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Aug 12, 2019
@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Aug 12, 2019
@SteveL-MSFT
Copy link
Member

@KirkMunro, I'll add you (and @vexx32) to the approved list. I should probably also allow restart as a verb.

@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Aug 21, 2019
@KirkMunro
Copy link
Contributor Author

FYI, the Codacy issue that is being flagged is a false positive.

@KirkMunro
Copy link
Contributor Author

Any chance we can get some progress on the pending reviews here? As indicated, the Codacy issue is a false positive. This PR is good to go, just needs reviews finalized so that it can be merged. Other work is pending the merge of this PR.

@adityapatwardhan adityapatwardhan merged commit cc0fed4 into PowerShell:master Sep 5, 2019
@adityapatwardhan adityapatwardhan added the CL-Experimental Indicates that a PR should be marked as an Experimental Feature in the Change Log label Sep 5, 2019
@adityapatwardhan adityapatwardhan added this to the 7.0.0-preview.4 milestone Sep 5, 2019
@adityapatwardhan
Copy link
Member

@KirkMunro Thank you for your contribution!

@TravisEz13 TravisEz13 added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Sep 5, 2019
@KirkMunro KirkMunro deleted the manage-breakpoints-in-runspace branch September 5, 2019 23:24
TravisEz13 pushed a commit to TravisEz13/PowerShell that referenced this pull request Sep 14, 2019
@anmenaga anmenaga removed the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Sep 17, 2019
@ghost
Copy link

ghost commented Sep 19, 2019

🎉v7.0.0-preview.4 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-Experimental Indicates that a PR should be marked as an Experimental Feature in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.