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.

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

conpty: request DSR-CPR before Win32 input mode #16445

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Dec 8, 2023

This prevents an issue in conhost where older versions of Windows Terminal (including the ones currently inbox in Windows, as well as stable and preview) will still cause WSL interop to hang on startup.

Since VT input is erroneously re-encoded as Win32 input events on those versions, we need to make sure we request the cursor position before enabling Win32 input mode. That way, the CPR we get back is properly encoded.

This prevents an issue in conhost where older versions of Windows
Terminal (including the ones currently inbox in Windows, as well as
stable and preview) will *still* cause WSL interop to hang on startup.

Since VT input is erroneously re-encoded as Win32 input events on those
versions, we need to make sure we request the cursor position *before*
enabling Win32 input mode. That way, the CPR we get back is properly
encoded.
@DHowett
Copy link
Member Author

DHowett commented Dec 8, 2023

From #16408

For this to be of any benefit, your proposed fix would assumedly have to make its way into the inbox conhost before Terminal Stable gets the #16407 fix. Am I understanding that correctly?

If that's a likely scenario, then it probably is worth doing. The only downside I can see is if the CPR response is slow, it might be possible that some keypresses arrive in VT format before win32-input mode kicks in. But that doesn't seem like a massive issue, assuming it's even possible at all.
@j4james, #14608

That's exactly it - we haven't even begun updating WT 1.18 and 1.19 in the store to fix this, but the conhost changes are making their way up out through the insider program. It's a narrow window where it'll be ~ ~ weird ~ ~, but this makes it smaller.

Good call about there being VT input in the wrong format on startup. It's probably safe - applications will hopefully not be expecting to receive raw modifier keypresses (🤞🏻) that early on . . .

@DHowett DHowett requested a review from lhecker December 8, 2023 20:43
Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this!

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well yea this is obvious in retrospect

@DHowett DHowett enabled auto-merge (squash) December 8, 2023 20:52
@DHowett DHowett merged commit 17867af into main Dec 8, 2023
18 of 20 checks passed
@DHowett DHowett deleted the dev/duhowett/fix-wsl-startup-deadlock-ii branch December 8, 2023 21:01
DHowett added a commit that referenced this pull request Dec 8, 2023
This prevents an issue in conhost where older versions of Windows
Terminal (including the ones currently inbox in Windows, as well as
stable and preview) will *still* cause WSL interop to hang on startup.

Since VT input is erroneously re-encoded as Win32 input events on those
versions, we need to make sure we request the cursor position *before*
enabling Win32 input mode. That way, the CPR we get back is properly
encoded.

(cherry picked from commit 17867af)
Service-Card-Id: 91301135
Service-Version: 1.19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants