Switch all DSR responses to appending instead of prepending #7583
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This fixes an issue where two CPRs could end up corrupted in the input buffer. An application that sent two CPRs back-to-back could accidentally read the first few characters of the first prepended CPR before handing us another CPR. We would dutifully prepend it to the buffer, causing them to overlap. ``` ^[^[2;2R[1;1R ^^ ^^^^^ First CPR ^^^^^^ Second CPR ``` Response prepending was implemented in !997738 without much comment. There's very little in the way of audit trail as to why we switched. Michael believes that we wanted to make sure that applications got DSR responses immediately. I discussed our options with him, and he suggested that we could implement a priority queue in InputBuffer and make sure that "response" input was dispatched to a client application before any application- or user-generated input. This was deemed to be too much work. We decided that DSR responses getting top billing was likely to be a stronger guarantee than most terminals are capable of giving, and that we should be fine if we just switch it back to append. Fixes #1637.
ghost
added
Area-Input
|
I know @zadjii-msft is on leave, but he's been floating around and may be peripherally interested in this. 😄 |
ghost
requested review from
zadjii-msft,
carlos-zamora,
leonMSFT and
PankajBhojwani
DHowett
added
the
AutoMerge
|
Hello @DHowett! Because this pull request has the Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 8 hours, a condition that will be fulfilled in about 6 hours 10 minutes. No worries though, I will be back when the time is right! 😉 p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
@msftbot merge this in 1 hour |
|
Hello @DHowett! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
carlos-zamora
approved these changes
Sep 9, 2020
|
Great work! BTW, how soon will there be a fixed release? The milestone page says there would be a bug-fixed release "Push to Microsoft Store at end of week 3". 3rd week monthly? so I guess it's around Sep 19? |
DHowett
added a commit
that referenced
this pull request
Sep 18, 2020
This fixes an issue where two CPRs could end up corrupted in the input buffer. An application that sent two CPRs back-to-back could end up reading the first few characters of the first prepended CPR before handing us another CPR. We would dutifully prepend it to the buffer, causing them to overlap. ``` ^[^[2;2R[1;1R ^^ ^^^^^ First CPR ^^^^^^ Second CPR ``` The end result of this corruption is that a requesting application would receive an unbidden `R` on stdin; for vim, this would trigger replace mode immediately on startup. Response prepending was implemented in !997738 without much comment. There's very little in the way of audit trail as to why we switched. Michael believes that we wanted to make sure that applications got DSR responses immediately. It had the unfortunate side effect of causing subsequence CPRs across cursor moves to come out in the wrong order. I discussed our options with him, and he suggested that we could implement a priority queue in InputBuffer and make sure that "response" input was dispatched to a client application before any application- or user-generated input. This was deemed to be too much work. We decided that DSR responses getting top billing was likely to be a stronger guarantee than most terminals are capable of giving, and that we should be fine if we just switch it back to append. Thanks to @k-takata, @Tekki and @brammool for the investigation on the vim side. Fixes #1637. (cherry picked from commit cb037f3)
|
🎉 Handy links: |
ghost
mentioned this pull request
|
🎉 Handy links: |
|
Thank you so much for this, you guys are the best ! ❤️ |
ArthurSonzogni
added a commit
to ArthurSonzogni/FTXUI
that referenced
this pull request
Jul 4, 2021
I finally got access to a computer using the Microsoft's Windows OS. That's the opportunity to find and mitigate all the problems encountered. This patch: 1. Introduce an option and a C++ definition to enable fallback for Microsoft's terminal emulators. This allows me to see/test the Microsoft output from Linux. This also allows Windows users to remove the fallback and target non Microsoft terminals on Windows if needed. 2. Microsoft's terminal suffer from a race condition bug when reporting the cursor position: microsoft/terminal#7583. The mitigation is not to ask for the cursor position in fullscreen mode where it isn't really needed and request it less often. This fixes: #136 3. Microsoft's terminal do not handle properly hidding the cursor. Instead the character under the cursor is hidden, which is a big problem. As a result, we don't enable setting the cursor to the best position for [input method editors](https://en.wikipedia.org/wiki/Input_method), It will be displayed at the bottom right corner. See: - microsoft/terminal#1203 - microsoft/terminal#3093 4. Microsoft's terminals do not provide a way to query if they support colors. As a fallback, assume true colors is supported. See issue: - microsoft/terminal#1040 This mitigates: - #135 5. The "cmd" on Windows do not properly report its dimension. Powershell works correctly. As a fallback, use a 80x80 size instead of 0x0. 6. There are several dom elements and component displayed incorrectly, because the font used is missing several unicode glyph. Use alternatives or less detailled one as a fallback.
ArthurSonzogni
added a commit
to ArthurSonzogni/FTXUI
that referenced
this pull request
Jul 4, 2021
I finally got access to a computer using the Microsoft's Windows OS. That's the opportunity to find and mitigate all the problems encountered. This patch: 1. Introduce an option and a C++ definition to enable fallback for Microsoft's terminal emulators. This allows me to see/test the Microsoft output from Linux. This also allows Windows users to remove the fallback and target non Microsoft terminals on Windows if needed. 2. Microsoft's terminal suffer from a race condition bug when reporting the cursor position: microsoft/terminal#7583. The mitigation is not to ask for the cursor position in fullscreen mode where it isn't really needed and request it less often. This fixes: #136 3. Microsoft's terminal do not handle properly hidding the cursor. Instead the character under the cursor is hidden, which is a big problem. As a result, we don't enable setting the cursor to the best position for [input method editors](https://en.wikipedia.org/wiki/Input_method), It will be displayed at the bottom right corner. See: - microsoft/terminal#1203 - microsoft/terminal#3093 4. Microsoft's terminals do not provide a way to query if they support colors. As a fallback, assume true colors is supported. See issue: - microsoft/terminal#1040 This mitigates: - #135 5. The "cmd" on Windows do not properly report its dimension. Powershell works correctly. As a fallback, use a 80x80 size instead of 0x0. 6. There are several dom elements and component displayed incorrectly, because the font used is missing several unicode glyph. Use alternatives or less detailled one as a fallback.
|
For those of you who are still following along at home, this fix was too late for Windows 10. Windows Server 2022 and Windows 11 both have the fix and the SSH server will operate properly. We're looking at our options for making these kinds of fixes available to all versions of Windows in the future. 🙂 |
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes an issue where two CPRs could end up corrupted in the input
buffer. An application that sent two CPRs back-to-back could
end up reading the first few characters of the first prepended CPR
before handing us another CPR. We would dutifully prepend it to the
buffer, causing them to overlap.
The end result of this corruption is that a requesting application
would receive an unbidden
Ron stdin; for vim, this would triggerreplace mode immediately on startup.
Response prepending was implemented in !997738 without much comment.
There's very little in the way of audit trail as to why we switched.
Michael believes that we wanted to make sure that applications got DSR
responses immediately. It had the unfortunate side effect of causing
subsequence CPRs across cursor moves to come out in the wrong order.
I discussed our options with him, and he suggested that we could
implement a priority queue in InputBuffer and make sure that "response"
input was dispatched to a client application before any application- or
user-generated input. This was deemed to be too much work.
We decided that DSR responses getting top billing was likely to be a
stronger guarantee than most terminals are capable of giving, and that
we should be fine if we just switch it back to append.
Thanks to @k-takata, @Tekki and @brammool for the investigation on the
vim side.
Fixes #1637.