Implementation for Invoke-Command step-in remote debugging #3015
Conversation
Minor updates to test. Added It:Skip for Windows only test. More CR changes Fix for SSH remoting Ctrl+C operation Fixed comment Clean up PSObject base Implementation for Invoke-Command step-in remote debugging Remove GP cache changes Added test Fixed potential null reference error Fixed ad hoc bug Added debugger ready wait in debugger process loop Stability improvements Improve step out behavior Added event raised when internal runsapce debug processing ends Added consistent runspace clean up Some clean up
| return | ||
| } | ||
|
|
||
| $sb = [scriptblock]::Create(@' |
There was a problem hiding this comment.
The below code should be moved to else
if (!$IsWindows)
{
$originalDefaultParameterValues = $PSDefaultParameterValues.Clone()
$PSDefaultParameterValues["it:skip"] = $true
} else {
$sb = [scriptblock]::Create(@'
"Hello!"
'@)
Add-Type -TypeDefinition $typeDef -ReferencedAssemblies "System.Globalization","System.Management.Automation"
...
}| if (!$IsWindows) | ||
| { | ||
| $global:PSDefaultParameterValues = $originalDefaultParameterValues | ||
| return |
There was a problem hiding this comment.
Th same - 'return' should be removed and below code should be moved to else .
| if ($remoteSession -ne $null) { Remove-PSSession $remoteSession -ErrorAction SilentlyContinue } | ||
| } | ||
|
|
||
| It "Verifies that asynchronous Invoke-Command -RemoteDebug is ignored" { |
There was a problem hiding this comment.
It would be better:
It "Verifies that asynchronous 'Invoke-Command -RemoteDebug' is ignored" { It would be good to fix headers below similarly.
| public override void StopProcessCommand() { } | ||
| } | ||
| } | ||
| '@ |
There was a problem hiding this comment.
If we skip all test for non-Windows the above code should be moved in
if ($IsWindows) {
# here
}| ## | ||
| ## PowerShell Invoke-Command -RemoteDebug Tests | ||
| ## | ||
|
|
There was a problem hiding this comment.
I don't know whether this work on IoT and Windows Core Server? If not, then we need to change the 'skipping'.
There was a problem hiding this comment.
The test works on Windows Core Server. I don't know about IoT
There was a problem hiding this comment.
I can point you to an IoT VM (internally) so you can at least run a smoke test there. IoT and Nano Server are very similar environments, so I would expect it to work there as well.
There was a problem hiding this comment.
I don't believe tests are currently run on IoT or Nano Server. So I feel this shouldn't block the PR merge.
There was a problem hiding this comment.
This comment doesn't block the PR merge.
But I do think for features like this one, we should at least verify if it works on NanoServer, because it adds great value to customers and thus is very possible to be cherry-picked to the NanoServer in-box powershell.
| ## | ||
| ## PowerShell Invoke-Command -RemoteDebug Tests | ||
| ## | ||
|
|
There was a problem hiding this comment.
I don't know whether this work on IoT and Windows Core Server? If not, then we need to change the 'skipping'.
There was a problem hiding this comment.
The test works on Windows Core Server. I don't know about IoT
… ICMRemoteDebug
| /// <summary> | ||
| /// The runspace to process | ||
| /// </summary> | ||
| public Runspace Runspace |
There was a problem hiding this comment.
5 lines for a trivial property is too verbose - please use one line.
| /// When set to true this will cause PowerShell to handle this runspace debug session internally through its | ||
| /// internal script debugger | ||
| /// </summary> | ||
| public bool HandleInternally |
There was a problem hiding this comment.
If this was an internal api, I wouldn't care about the name as much, but it's public, so can you think of a better term than Internally to prevent any ambiguity as to what Internally actually refers to?
There was a problem hiding this comment.
Changed to "UseDefaultProcessing"
| /// <summary> | ||
| /// Event raised when a runspace debugger needs breakpoint processing | ||
| /// </summary> | ||
| public event EventHandler<ProcessRunspaceDebugEventArgs> ProcessRunspaceDebug; |
There was a problem hiding this comment.
I'm finding Process confusing here - it's not the noun, but it seems like maybe RunspaceDebugProcessingStarted, RunspaceDebugProcessingCompleted, and RunspaceDebugProcessingCancelled might be clearer.
There was a problem hiding this comment.
This is a bit confusing. I'll change "ProcessRunspaceDebugEnd" to "RunspaceDebugProcessingCompleted" since that is much better.
The other two should probably be "StartRunspaceDebugProcessing" and "CancelRunspaceDebugProcessing".
The reason these two events exist is to give a host the chance to debug these remote sessions directly , rather than let PowerShell provide a default debug experience. For example VSCode may want to show multiple windows to debug multiple remote sessions simultaneously instead of using PowerShell default processing which debugs each session serially.
| /// <summary> | ||
| /// Event raised when debugging session is over and runspace debuggers queued for processing should be released | ||
| /// </summary> | ||
| public event EventHandler<EventArgs> CancelProcessRunspaceDebug; |
There was a problem hiding this comment.
The comment suggests this event is raised under normal processing but Cancel suggests termination that might not be normal.
| /// <param name="runspace">Runspace to debug</param> | ||
| internal override void QueueRunspaceForDebug(Runspace runspace) | ||
| { | ||
| if (runspace == null) { throw new PSArgumentNullException("runspace"); } |
There was a problem hiding this comment.
Do we really need argument validation here? The method is internal, so it seems unnecessary - PSArgumentNullException or NullReferenceException are equally informative.
| public void PushRunspace(Runspace runspace) { } | ||
| public void PopRunspace() { } | ||
| public bool IsRunspacePushed { get { return false; } } | ||
| public Runspace Runspace { get { return _runspace; } private set { _runspace = value; } } |
There was a problem hiding this comment.
It's just test code - but won't this return the wrong runspace if/when PushRunspace is used?
There was a problem hiding this comment.
The test host Push/Pop methods are not needed for the test. Only the Runspace property is needed to provide access to the host runspace.
There was a problem hiding this comment.
We should avoid incorrect code where possible - it has a nasty habit of getting copied.
There was a problem hiding this comment.
This is not incorrect code. The methods are simply unnecessary for the test.
There was a problem hiding this comment.
We are trying to apply the same standard between test code and product code.
If this code were copied or reused beyond this test (and people definitely do copy our test code - maybe treating it like a sample) - what change would you suggest to avoid problems?
Maybe throw from push/pop - knowing that Runspace does not work as intended with push/pop?
At a minimum, I'd say you would add a comment pointing out the flaw when the code is used elsewhere.
| if (!$IsWindows) | ||
| { | ||
| $originalDefaultParameterValues = $PSDefaultParameterValues.Clone() | ||
| $PSDefaultParameterValues["it:skip"] = $true |
There was a problem hiding this comment.
This feature only works on Windows? Is that a temporary situation (I hope), then shouldn't this be Pending, not Skip?
|
|
||
| It "Verifies that asynchronous 'Invoke-Command -RemoteDebug' is ignored" { | ||
|
|
||
| $ps.AddCommand("Invoke-Command").AddParameter("Session", $remoteSession).AddParameter("ScriptBlock", $sb).AddParameter("RemoteDebug", $true).AddParameter("AsJob", $true) |
There was a problem hiding this comment.
Lines like this are hard to read if on one line. You can move method calls to the next line like this:
$ps.AddCommand("Invoke-Command").
AddParameter("Session", $remoteSession).
AddParameter ...| #region Runspace Debug Processing | ||
|
|
||
| /// <summary> | ||
| /// RaiseProcessRunspaceDebugEvent |
There was a problem hiding this comment.
Please use helpful xml doc comments or use a simple:
/// <summary/>to silence the compiler warnings.
| @@ -2938,6 +3189,10 @@ private Debugger PopActiveDebugger() | |||
| _steppingMode = SteppingMode.StepIn; | |||
| _overOrOutFrame = _nestedRunningFrame; | |||
| _nestedRunningFrame = null; | |||
| if (_lastActiveDebuggerAction == DebuggerResumeAction.StepOut) | |||
| { | |||
| CancelDebuggerProcessingAsNeeded(poppedDebugger); | |||
There was a problem hiding this comment.
Again, I'm finding the verb used confusing. Here stepping out is normal - so do we really mean to "cancel" debugger processing?
There was a problem hiding this comment.
To elaborate, I expect potentially different cleanup code if something completes normally versus terminated (cancelled) for some other reason. Maybe they aren't different in this code, but that's how I'm thinking about it.
There was a problem hiding this comment.
The CancelDebuggerProcessingAsNeeded() method cancels debugger processing only when stepping out of the current call stack, which means stepping out of the currently debugged remote session. This behavior was debated but we finally decided to step out of all remote sessions from the Invoke-Command, and stop in the local script that called Invoke-Command.
We decided to do this because stepping out of one remote session and into another results in confusion because you are debugging one remote session while one or more are running in parallel spewing output into your debug session console. So "step-out" at the top of the stack means to step out of the Invoke-Command remote debugging and back to the local script.
I'll add a comment to try to make this clear.
There was a problem hiding this comment.
Thinking about this some more I have changed my mind. Right now "step-out" aborts the Invoke-Command debugging and continues with local script debugging, which I thought was the right behavior for some reason. But this is different behavior from how "continue" and "quit" debugger commands work. I think "step-out" should be consistent and I'll remove this cancel code.
| if (-not (Get-Module TestRemoting -ErrorAction SilentlyContinue)) | ||
| { | ||
| $remotingModule = Join-Path $PSScriptRoot "../Common/TestRemoting.psm1" | ||
| Import-Module $remotingModule |
There was a problem hiding this comment.
No need to check Get-Module TestRemoting
This work well:
Import-Module $remotingModule -ErrorAction SilentlyContinue|
|
||
| It "Verifies that s 179B ynchronous 'Invoke-Command -RemoteDebug' invokes debugger" { | ||
|
|
||
| $ps.Commands.Clear() |
There was a problem hiding this comment.
If somebody add a new test with AddCommand this may break the existing tests.
It would be safer to move $ps.Commands.Clear() in AfterEach {}
There was a problem hiding this comment.
Good idea. Thanks.
|
@PaulHigin Whether to increase the number of files with similar tests? |
|
@iSazonov I feel it is not necessary at this point. If in the future more tests rely on these classes then the code can be refactored. |
|
@PaulHigin Thanks for clarification! Clear. |
|
@mirichmo With regard to the tests the minor question remains open about IoT #3015 (review)
|
|
@daxian-dbw This PR has been languishing for weeks. Is there any reason why it cannot be merged? It seems to me that any minor issues can be addressed as they come up. Otherwise I am Ok abandoning this PR since this a debugging feature requested internally. |
|
I think this should be merged, it adds value for all our users, not just the internal ones. This functionality will light up for users in VS Code once it's released in PowerShell Core. |
|
Most of my feedback was addressed. I won't block merging, but do think my feedback on the host test code should be addressed - I expanded on my thinking inline. |
|
I opened #3236 to capture 2 open comments in the review. Given that, I'm merging this PR. |
These changes provide the ability to debug remote running scripts started with the Invoke-Command cmdlet. The design is event based and provides new public events that allow subscribers to be notified when an Invoke-Command remote session is ready for debugging. Since Invoke-Command allows running scripts on multiple targets at once (fan-out) the notification event is raised for each remote session as it becomes ready for debugging. The subscriber to these events will be a script debugger implementation (such as PowerShell console, ISE, or VSCode) and will handle all debugging details such as simultaneously debugging multiple remote sessions at once in separate windows.
But these changes also include an internal implementation which is used by default if host debuggers don't want to handle the debugging details. This internal implementation is what PowerShell console, ISE uses so they can have this new behavior without having to modify their debugger implementations. The internal implementation serializes each remote session of Invoke-Command so that they can be debugged one at a time. The remote session debugger is "pushed" onto the internal debugger stack so that debugging transitions to the remote session. Existing debugging commands work so that the "quit" debugging command will stop the current remote session script from running and allow the next remote session to be debugged. Similarly the "continue" debugging command allows the script to continue running outside step mode and again go to the next remote session for debugging. The "stepout" debugging command steps out of all Invoke-Command remote sessions and lets the script continue to run for each remote session in parallel as they are normally run.
The purpose of Invoke-Command step-in remote debugging is allow seamless debugging of a local script that calls Invoke-Command on remote targets. But there is also a new Invoke-Command "-RemoteDebug" parameter that lets you Invoke-Command on the command line and have it drop directly into the debugger.
An example from the PowerShell command line looks like this:
Notice that the debugger "stepin" command transitioned from local script debugging to debugging the remote session on computer "paulhig-3", as can be seen by the change in the debugger prompt.
You can also do this from the command line to drop directly into the debugger
These changes also remove an old behavior that was incompatible with this new step-in feature. Previously if a remote session running script hit a break point it would stop in the debugger and go to the "disconnected session" state. This was to allow the user to reconnect using Enter-PSSession and then interactively debug the remote session script. This behavior has been removed and now the user needs to attach a debugger using the newer Debug-Runspace cmdlet.