Skip to content
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

Closing tab hangs when using VS Dev tools and Powershell Core #14032

Closed
phil-scott-78 opened this issue Sep 19, 2022 · 7 comments · Fixed by #14041
Closed

Closing tab hangs when using VS Dev tools and Powershell Core #14032

phil-scott-78 opened this issue Sep 19, 2022 · 7 comments · Fixed by #14041
Assignees
@lhecker
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@phil-scott-78
Copy link

Windows Terminal version

1.16.2524.0

Windows build number

10.0.25201.0

Other Software

  • Visual Studio 2022
  • Powershell Core 7.2.6

Steps to reproduce

  1. Create a profile using pwsh.exe in Windows Terminal Preview
  2. Execute the following lines to add the VS dev tools
Import-Module "C:\Program Files\Microsoft Visual Studio\2022\Preview\Common7\Tools\Microsoft.VisualStudio.DevShell.dll"
Enter-VsDevShell 8ee891c5 -SkipAutomaticLocation -DevCmdArguments "-arch=x64 -host_arch=x64"
  1. Close the tab

Expected Behavior

Tab closes quickly.

Actual Behavior

wt locks up entirely doing...something.

Animation

I've tested this with the release version of Windows Terminal and also regular powershell.exe. Both of those don't exhibit the behavior. It is only with Powershell Core, the most recent release of wt.exe and the Visual Studio dev tools
@phil-scott-78 phil-scott-78 added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Sep 19, 2022
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Sep 19, 2022
@DHowett
Copy link
Member

DHowett commented Sep 19, 2022

Ah, this could (possibly?) be fallout from #13882. /cc @lhecker for when he's got time.
Thanks for the report!

@lhecker lhecker self-assigned this Sep 19, 2022
@phil-scott-78
Copy link
Author

I suspect you are right, I did a few heap dumps to see if I could spot anything of note and I noticed they were both hung on the same line related to the conpty connect, specifically this line https://github.com/microsoft/terminal/blob/main/src/cascadia/TerminalConnection/ConptyConnection.cpp#L557

@lhecker
Copy link
Member

lhecker commented Sep 19, 2022 •
edited
Loading

I debugged this just now and and it's actually hung somewhere else for me:
(It's hung in WaitForSingleObject(pPty->hConPtyProcess, INFINITE);)

image

@lhecker
Copy link
Member

lhecker commented Sep 19, 2022 •
edited
Loading

Ah right... The old version used to close the connection in the background only, so it didn't matter how long it took and it wasn't visually apparent either: 666c446#diff-baa2e4fc26af1b8722a4eec13221622ef4ae566e4f7a8658bc58806f96b7e9b8L1525-L1526
But as mentioned in the PR, that's not viable at all, because that's a pretty nasty race condition.

@lhecker lhecker mentioned this issue Sep 19, 2022
Merged
@ghost ghost added the In-PR This issue has a related PR label Sep 19, 2022
@lhecker lhecker added Product-Conpty For console issues specifically related to conpty Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. Priority-1 A description (P1) labels Sep 19, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Sep 19, 2022
@phil-scott-78
Copy link
Author

@lhecker, I just double checked - I'm hung on the same line. The one I posted earlier was down further in the stack.

@ghost ghost closed this as completed in #14041 Sep 21, 2022
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Sep 21, 2022
ghost pushed a commit that referenced this issue Sep 21, 2022
@lhecker
Fix potential lags/deadlocks during tab close (#14041)
274bdb3 
`ConptyClosePseudoConsole` blocks until OpenConsole exits.
This is problematic for the changes in 666c446, which stopped calling that
function on a background thread to solve a race condition. This commit fixes
the potential lags/deadlocks from waiting on OpenConsole's exit, by adding
`ConptyClosePseudoConsoleNoWait` which only closes the IO handles and allows
OpenConsole to exit naturally. This uncovered another potential deadlock
in `ServiceLocator::RundownAndExit` which might call itself recursively.

Closes #14032

## Validation Steps Performed
* Print tons of text and concurrently close the tab.
  Tab closes, OpenConsole/pwsh exits instantly ✅
* Use `Enter-VsDevShell` and close the tab.
  Tab closes instantly, OpenConsole/pwsh exits after ~5 seconds ✅
DHowett pushed a commit that referenced this issue Sep 21, 2022
@lhecker @DHowett
Fix potential lags/deadlocks during tab close (#14041)
45310d7 
`ConptyClosePseudoConsole` blocks until OpenConsole exits.
This is problematic for the changes in 666c446, which stopped calling that
function on a background thread to solve a race condition. This commit fixes
the potential lags/deadlocks from waiting on OpenConsole's exit, by adding
`ConptyClosePseudoConsoleNoWait` which only closes the IO handles and allows
OpenConsole to exit naturally. This uncovered another potential deadlock
in `ServiceLocator::RundownAndExit` which might call itself recursively.

Closes #14032

## Validation Steps Performed
* Print tons of text and concurrently close the tab.
  Tab closes, OpenConsole/pwsh exits instantly ✅
* Use `Enter-VsDevShell` and close the tab.
  Tab closes instantly, OpenConsole/pwsh exits after ~5 seconds ✅

(cherry picked from commit 274bdb3)
Service-Card-Id: 85767341
Service-Version: 1.16
@ghost
Copy link

ghost commented Sep 23, 2022

🎉This issue was addressed in #14041, which has now been successfully released as Windows Terminal Preview v1.16.2641.0.:tada:

Handy links:

DHowett pushed a commit that referenced this issue Oct 13, 2022
@lhecker @DHowett
Fix potential lags/deadlocks during tab close (#14041)
0f94704 
`ConptyClosePseudoConsole` blocks until OpenConsole exits.
This is problematic for the changes in 666c446, which stopped calling that
function on a background thread to solve a race condition. This commit fixes
the potential lags/deadlocks from waiting on OpenConsole's exit, by adding
`ConptyClosePseudoConsoleNoWait` which only closes the IO handles and allows
OpenConsole to exit naturally. This uncovered another potential deadlock
in `ServiceLocator::RundownAndExit` which might call itself recursively.

Closes #14032

## Validation Steps Performed
* Print tons of text and concurrently close the tab.
  Tab closes, OpenConsole/pwsh exits instantly ✅
* Use `Enter-VsDevShell` and close the tab.
  Tab closes instantly, OpenConsole/pwsh exits after ~5 seconds ✅

(cherry picked from commit 274bdb3)
Service-Card-Id: 86209670
Service-Version: 1.15
@ghost
Copy link

ghost commented Oct 18, 2022

🎉This issue was addressed in #14041, which has now been successfully released as Windows Terminal v1.15.2874.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lhecker lhecker

Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix potential lags/deadlocks during tab close microsoft/terminal
3 participants
@DHowett @lhecker @phil-scott-78