Skip to content

Fix attach to process debugging #1736

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.

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

Closed
SeeminglyScience opened this issue Mar 1, 2022 · 5 comments · Fixed by #1752
Closed

Fix attach to process debugging #1736

SeeminglyScience opened this issue Mar 1, 2022 · 5 comments · Fixed by #1752
Assignees
Labels

Comments

@SeeminglyScience
Copy link
Collaborator

See PowerShell/vscode-powershell#3849

@SeeminglyScience SeeminglyScience self-assigned this Mar 1, 2022
@ghost ghost added the Needs: Triage Maintainer attention needed! label Mar 1, 2022
@JustinGrote
Copy link
Collaborator

As a "nice to have" on this, PSES could watch for a runspace in the PSIC to go to InBreakpoint state, and then advertise a new "child" debug session in vscode that enters at that breakpoint. How that would look in terms of the PSIC (or a new dummy PSIC or something) is TBD, but seems feasible once the foundation aspect of this is sorted.

@SeeminglyScience
Copy link
Collaborator Author

So what's kind of complicated about that, is if PSES isn't involved in the runspaces creation, we don't actually have much in the way of hooks.

iirc a runspace with the default host (literally DefaultHost, not the one from pwsh.exe) just straight up ignores breakpoints. On top of that, what do we do when multiple runspaces hit a breakpoint. Part of the issue is we don't have a way to globally freeze a process like dotnet does. There's no central tie in to runspaces like there is for threads in dotnet.

Can you tell I've thought about this a lot? 😁 I'd love to get to closer parity with how flawlessly omnisharp handles multiple threads, but we may very well need better support from the engine to do so.

@SeeminglyScience
Copy link
Collaborator Author

Currently we don't let go of control in PSHost.PushRunspace. This makes it challenging to pass another command, and also does not act the same way as ConsoleHost. I may need to restructure some things to get this working.

@JustinGrote
Copy link
Collaborator

JustinGrote commented Mar 3, 2022

At least on the multiple runspaces part, at the vscode level we can expose that as multiple debug sessions in the dropdown, but what gets tricky is handling the messaging there via omnisharp, yes. Also IMHO wouldn't be expected to freeze all of the session if one runspace goes into breakpointing, all the other runspaces would continue to operate. That would for sure make things like race conditions harder to troubleshoot etc. but I think most runspace usage in PS is fan-out non-related isolated scriptblocks (foreach -parallel, start-threadjob, and the like) so I don't think this is a big issue. 90% of the time that I've wanted this, it's because my threadjob isn't working and I wish I could just breakpoint it rather than have to throw in a wait-debugger or comment out or have a flag to say "don't do this in a runspace so I can debug it"

So I mean there's Get-Runspace, is there not a C# level way of hooking into that info?

@SeeminglyScience
Copy link
Collaborator Author

SeeminglyScience commented Mar 3, 2022

but what gets tricky is handling the messaging there via omnisharp, yes.

Oops my bad! By omnisharp I meant more the CLR's debugging infrastructure. Poor choice of words on my part since we also happen to use the omnisharp team's LSP implementation 😁 (and also because omnisharp.exe is really just a wrapper around the infrastructure I'm talking about anyway (ICorDebug))

Also IMHO wouldn't be expected to freeze all of the session if one runspace goes into breakpointing, all the other runspaces would continue to operate.

That's definitely how I would want it. It would be the only safe way to examine the state of the other runspaces at the same time, and would prevent the inevitable issue of multiple runspaces hitting breakpoints at the same time.

That would for sure make things like race conditions harder to troubleshoot etc. but I think most runspace usage in PS is fan-out non-related isolated scriptblocks (foreach -parallel, start-threadjob, and the like) so I don't think this is a big issue. 90% of the time that I've wanted this, it's because my threadjob isn't working and I wish I could just breakpoint it rather than have to throw in a wait-debugger or comment out or have a flag to say "don't do this in a runspace so I can debug it"

I would argue that's not too dissimilar to the use case of threads in C# either. Often many of the threads are only vaguely related to each other, but stopping all execution to examine one is still critical imo.

So I mean there's Get-Runspace, is there not a C# level way of hooking into that info?

There's no event that triggers saying that a runspace is created. Like we could run Get-Runspace to figure out what's out there, but if a runspace is created mid script we would have no hooks into that process.

@andyleejordan andyleejordan removed the Needs: Triage Maintainer attention needed! label Mar 15, 2022
SeeminglyScience added a commit to SeeminglyScience/PowerShellEditorServices that referenced this issue Apr 11, 2022
Fixes PowerShell#1736

- Add a new context frame for runspace entrance and REPL loops
- Ensure runspace access is not attempted on a broken runspace
- When a remote runspace hits a debugger stop, ensure that the
  origin pipeline thread does not run a REPL concurrently
- Fix many race conditions
@andyleejordan andyleejordan changed the title Fix attach to process Fix attach to process debugging Apr 18, 2022
andyleejordan pushed a commit that referenced this issue Apr 18, 2022
* Fix an occassional dead lock on restarting debug
* Fix attach to process and other debugging issues
* Fixes #1736
  * Add a new context frame for runspace entrance and REPL loops
  * Ensure runspace access is not attempted on a broken runspace
  * When a remote runspace hits a debugger stop, ensure that the
     origin pipeline thread does not run a REPL concurrently
  * Fix many race conditions
* Use FilterText instead of Label for Assert
   * If multiple Write-Host's exist then this test will fail. FilterText
     contains the module qualified name when dupes are present.
* Use Contains instead of Collection in assert
  * If the testing machine has a pester template installed the collection
    will have an extra item.
* Add some safety to disposals
* Skip CLM E2E tests in unelevated process
* Add preset ExecutionOptions.ImmediateInteractive
* Cleanup extra null conditionals
* Add comment describing disconnect code path
* Add `Is*` properties for context frame types
* Use the new `Is*` props in a few more places
* Fix new analyzers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants