Re-enable DebugServiceTests suite

[andyleejordan]

Please see commits for work done.

[JustinGrote]

Pushed a commit to the wrong branch, just reverted it, not a good look for my first day on the job :P

[andyleejordan]

@JustinGrote Progress! The first two three tests are passing.

[JustinGrote]

@andschwa great! Once this gets merged I'll rebase my existing PRs and try to add tests accordingly.

[andyleejordan]

@JustinGrote Hm, everything but the four that I've skipped (and they're unfortunately indicative of real broken things) pass on my Mac...but not in Windows CI 😢

[andyleejordan]

The common failure is around the event not getting added to the debugger stopped queue, which is what breaks the first skipped test and I've been debugging. I believe there's a race condition in the debugger or its handlers. Working on finding it. @SeeminglyScience if you have any ideas, let me know!

[andyleejordan]

@SeeminglyScience @JustinGrote I found the deadlock! In the tests, any code that calls into debugService can deadlock. On PS Core this only appeared with debugService.Break(): calling that API essentially requires the PSES pipeline thread to produce some events which eventually push to our debugger stopped queue, and calling it puts us into the pipeline thread. Wrapping it in a Task.Run() is a viable solution because it lets the test code return to a non pipeline thread. On Windows PowerShell the problem becomes far more evident because the PS Core breakpoint APIs are not available, and the "legacy" fallback code then runs into the same problem, so SetLine/CommandBreakpointAsync tosses us onto the pipeline thread. These are async tasks and are fixed by using ConfigureAwait(true), which says "return us to the captured thread context." This actually makes sense given that the tests are essentially UI code, I may just replace all ConfigureAwait(false) in test code with true.

Unfortunately there's still a large hill (but not a mountain any more) of work here: I need to clean all of this up for one, and for two, the set variable logic is broken and the associated tests currently skipped. I might punt the latter.

[JustinGrote]

@andschwa look at you with your green checkmarks :). Thanks for what I'm sure was a very perplexing effort.

[andyleejordan]

@rjmholt, Dongbo and I weren't sure what this logic should be. Right now, we're changing it slightly to compare the previous runspace against the previous PowerShell frame's runspace, which seemingly fixes what we think was an oversight. I've stashed a change that reverts the logic to compare to the new PowerShell frame's runspace, except with a check if there are no more PowerShell frames. We'll see what happens!

[SeeminglyScience]

It seems like it's trying to check if popping the current frame puts us into a different runspace than before.

e.g.

1. Process start, default frame
2. Enter-PSSession, new remote frame
3. Exit-PSSession, pop frame back to 1
4. Check CurrentPowerShell, see if we've moved to a different runspace.

[daxian-dbw]

LGTM. @andschwa walked me through the changes, thanks Andy!

[SeeminglyScience]

Awesome work! A couple of mostly small things

[SeeminglyScience]

It seems like it's trying to check if popping the current frame puts us into a different runspace than before.

e.g.

1. Process start, default frame
2. Enter-PSSession, new remote frame
3. Exit-PSSession, pop frame back to 1
4. Check CurrentPowerShell, see if we've moved to a different runspace.

[andyleejordan]

@SeeminglyScience For some reason I can't reply to this:

#1635 (comment)

I get that's what it's trying to do, but I'm having trouble following the logic. The existing logic runs into a null dereference issue, because first a frame is popped, and then we do a peek on the frame stack. If this logic is correct it needs to be adjusted for the case where the stack is empty after that pop. The current change in this PR assumes that the existing logic was an oversight and that it was intending to compare to the frame being popped. What do you think? Dongbo and I had decided just to test one, see what happens, and change it if needed 😅

[andyleejordan]

@daxian-dbw Upon examination, yes your suspicion is correct: xUnit adds its own synchronization context even if parallelism is disabled, since we're using async test methods:

https://github.com/xunit/xunit/blob/7f5286f6587fa0b600c9bdebef949dff5301c83e/src/xunit.v3.core/Sdk/AsyncTestSyncContext.cs

[SeeminglyScience]

If this logic is correct it needs to be adjusted for the case where the stack is empty after that pop. The current change in this PR assumes that the existing logic was an oversight and that it was intending to compare to the frame being popped. What do you think?

Is it possible that PopPowerShell is being called too many times? Either way I think this change is going to make the runspace changed determination lag behind by one frame if that makes sense.

Unless I'm missing something, right now it's checking if the current frame is on the current runspace, but that'll always be the case. I think the current fix is maybe hiding a different bug.

[andyleejordan]

Is it possible that PopPowerShell is being called too many times? Either way I think this change is going to make the runspace changed determination lag behind by one frame if that makes sense.

I don't think so. The case where it's an empty stack is an easy one: when the last PowerShell frame is being popped, the stack is now empty. CurrentPowerShell.Runspace then does _psFrameStack.Peek() on said empty stack.

Unless I'm missing something, right now it's checking if the current frame is on the current runspace, but that'll always be the case. I think the current fix is maybe hiding a different bug.

This I agree with, it doesn't seem right to me now. Potential fix:

if (_psFrameStack.Count != 0 && previousRunspaceFrame.Runspace != _psFrameStack.Peek().PowerShell.Runspace)

[SeeminglyScience]

This I agree with, it doesn't seem right to me now. Potential fix:

Something still seems a little off. I think there should always be something on the stack unless I'm remembering wrong. Is it possible this only happens at tear down? And just a lot more noticeable in tests since it's happening more often?

If that's the case then yeah that seems like a good fix. If it's not only during tear down then I'm a little confused how it can function after PopPowerShell exits with an empty stack.

Edit: oh that might be what you're saying in the first part there?

[andyleejordan]

@SeeminglyScience I stepped through it a few more times and I'm convinced the null-dereference is happening during cleanup, and so was an overlooked edge case. Here's what I'm about to rebase into this PR:

        private void PopPowerShell(RunspaceChangeAction runspaceChangeAction = RunspaceChangeAction.Exit)
        {
            _shouldExit = false;
            PowerShellContextFrame frame = _psFrameStack.Pop();
            try
            {
                // If we're changing runspace, make sure we move the handlers over. If we just
                // popped the last frame, then we're exiting and should pop the runspace too.
                if (_psFrameStack.Count == 0
                    || _runspaceStack.Peek().Runspace != _psFrameStack.Peek().PowerShell.Runspace)
                {
                    RunspaceFrame previousRunspaceFrame = _runspaceStack.Pop();
                    RemoveRunspaceEventHandlers(previousRunspaceFrame.Runspace);

                    // If there is still a runspace on the stack, then we need to re-register the
                    // handlers. Otherwise we're exiting and so don't need to run 'RunspaceChanged'.
                    if (_runspaceStack.Count > 0)
                    {
                        RunspaceFrame newRunspaceFrame = _runspaceStack.Peek();
                        AddRunspaceEventHandlers(newRunspaceFrame.Runspace);
                        RunspaceChanged?.Invoke(
                            this,
                            new RunspaceChangedEventArgs(
                                runspaceChangeAction,
                                previousRunspaceFrame.RunspaceInfo,
                                newRunspaceFrame.RunspaceInfo));
                    }
                }
            }
            finally
            {
                frame.Dispose();
            }
        }