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

Windows terminal as default terminal application breaks WSL2 launch when WSL2 debugConsole is enabled #10194

Closed
reynoldsa opened this issue May 26, 2021 · 4 comments · Fixed by #10415
Closed

Windows terminal as default terminal application breaks WSL2 launch when WSL2 debugConsole is enabled #10194

reynoldsa opened this issue May 26, 2021 · 4 comments · Fixed by #10415
Assignees
@miniksa
Labels
Area-DefApp Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v2.0

Comments

@reynoldsa
Copy link
Contributor

reynoldsa commented May 26, 2021 •
edited
Loading

Windows Terminal version (or Windows build number)

1.9.1445.0

Other Software

  1. WSL2
  2. Windows 10 build 21387 or later, for the WSL2 debugConsole functionality

Steps to reproduce

  1. Install WSL2 + a distro and create the %USERPROFILE%\.wslconfig file with the following contents:
[wsl2]
debugConsole=true
  1. Make sure that Terminal v1.9.1445.0 is installed, and that it's a) set as the default terminal b) has a profile for your WSL2 distro
  2. Attempt to launch terminal in the WSL2 profile, either via the jump list or terminal's own profile menu

Expected Behavior

  1. WSL2 profile opens normally

Actual Behavior

  1. Windows terminal locks up and goes into a "not responding" state, with high system CPU usage from VM compute service
  2. After the windows terminal process is killed, a legacy console window spawns with the expected WSL2 VM debug output
  3. After the VM boots, another Windows terminal spawns without any tabs open, lacking any WSL profile, and doesn't respond to window close events
  4. When the terminal instance from 3 is killed, and another one is spawned, this one will work normally
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels May 26, 2021
@zadjii-msft zadjii-msft added Area-DefApp Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. labels May 26, 2021
@zadjii-msft zadjii-msft added this to the Terminal v2.0 milestone May 26, 2021
@zadjii-msft zadjii-msft added the Priority-1 A description (P1) label May 26, 2021
@rescenic

This comment has been minimized.

Sign in to view
@DHowett

This comment has been minimized.

Sign in to view
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label May 27, 2021
@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label May 27, 2021
@miniksa
Copy link
Member

miniksa commented Jun 9, 2021

My proposed fix idea for #10134 fixes this too.

@miniksa miniksa mentioned this issue Jun 11, 2021
Merged
7 tasks
@ghost ghost added the In-PR This issue has a related PR label Jun 11, 2021
@ghost ghost closed this as completed in #10415 Jun 16, 2021
@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 Jun 16, 2021
ghost pushed a commit that referenced this issue Jun 16, 2021
@miniksa
Persist inbox conhost; delegate control activities to it via a pipe (#…
2bd5791 
…10415)

Persist inbox conhost; delegate control activities to it via a pipe

## PR Checklist
* [x] Closes #10194 - WSL Debug Tap doesn't work
* [x] Closes #10134 - WSL Parameter is Incorrect
* [x] Closes #10413 - Ctrl+C not passed to client
* [x] Closes #10414 - Leftover processes on abrupt termination
* [x] Might help #10251 - Win+X Powershell sometimes fails to attach
* [x] I work here
* [x] Manually tested with assorted launch scenarios

## Detailed Description of the Pull Request / Additional comments
It turns out that there's a bit of ownership that goes on with the original inbox `conhost.exe` and the operating system/driver. The PID of that original `conhost.exe` is stowed when the initial connection is established and it is verified for several activities. This means that the plan of letting it go completely away and having the `OpenConsole.exe` take over all of its activities must be slightly revised. 

I have tested the following two alternatives to keeping `conhost.exe` around and they do not work:
1. Replacing the original owner `conhost.exe` with `OpenConsole.exe` - A.) The driver does not allow this. Once the owner is registered, it cannot be replaced. B.) There's no way of updating this information inside the client process space and it is kept there too in the `kernelbase`/`conclnt` data from its initial connection.
2. Attempting to pick up the first packet (to determine headed/headless and other initial connection information that we use to determine whether handoff is appropriate or not) prior to registering any owner at all. - The driver doesn't allow this either. The owner must be registered prior to a packet coming through.

Put this mental model in your head:
CMD --> Conhost (inbox) --> OpenConsole (WT Package) --> Terminal (WT Package)

So since the `conhost.exe` needs to stick around, here's what I'm doing in this PR:
- `conhost.exe` in the OS will receive back the `OpenConsole.exe` process handle on a successful handoff and is expected to remain alive until the `OpenConsole.exe` exits. It's now waiting on that before it terminates itself.
- `conhost.exe` in the OS will establish a signal channel pipe and listen for control commands from `OpenConsole.exe` in a very similar fashion to how the `ConPTY` signal pipe operates between the Terminal and the PTY (provided by `OpenConsole.exe` in this particular example.) When `OpenConsole.exe` needs to do something that would be verified by the OS and rejected... it will instead signal the original `conhost.exe` to do that thing and it will go through.
- `conhost.exe` will give its own handle through to `OpenConsole.exe` so it can monitor its lifetime and cleanup. If the owner is gone, the session should end.
- Assorted handle cleanup that was leading to improper exits. I was confused between `.reset()` and `.release()` for some of the `wil::unique_any<T>` handling and it lead to leaked handles. The leaked handles meant that threads weren't aware of the other sides collapsing and wouldn't cleanup/terminate appropriately.

How does this fix things?
- For the WSL cases... WSL was specifically looking up the owner PID of the console session from the driver. That was the `conhost.exe` PID. If it exits, that PID isn't valid and is recycled. Thus the parameter is incorrect or other inappropriate WSL setup behaviors.
- Ctrl+C not passed... this is a signal the operating system rejects from a PID that is not the owner. This is now relayed through the original owner and it works.
- Leftover processes... I believe I explained this was both not-enough-monitoring of each others' process lifetimes coupled with mishandling of release/resetting handles and leaking them.
- Powershell sometimes fails to attach... my theory on this one is that it's a race that became upset when the `conhost.exe` disappeared while something about Powershell/.NET was still starting, much like the WSL one. I believe now that it is sticking around, it will be fine.

Also, this WILL require an OS update to complete improvement of functionality and I have revised the interface ID. This is considered an acceptable breaking change with no mitigation because we said this feature was an alpha preview.  

## Validation Steps Performed
- Launched WSL with defapp set, it works
- Launched WSL with defapp set and the debug tap on, it works and opens in two tabs
- Launched CMD, ran ping, did Ctrl+C, it now receives it
- Launched Win+X powershell a ton of times. It seems fine now
- Launched cmd, powershell, wsl, etc. Killed assorted processes in the chain (client/conhost/openconsole/windowsterminal) and observed in Process Explorer (with a long delta timer so I could see it) that they all successfully tear down now without leftovers.
@ghost
Copy link

ghost commented Jul 14, 2021

🎉This issue was addressed in #10415, which has now been successfully released as Windows Terminal Preview v1.10.1933.0.: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

@miniksa miniksa

Labels
Area-DefApp Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) 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
Terminal v2.0
Development

Successfully merging a pull request may close this issue.

Persist inbox conhost; delegate control activities to it via a pipe microsoft/terminal
5 participants
@DHowett @reynoldsa @rescenic @miniksa @zadjii-msft