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 "pass-through" doesn't maintain order of operations #8698

Closed
j4james opened this issue Jan 3, 2021 · 11 comments · Fixed by #17510
Closed

Conpty "pass-through" doesn't maintain order of operations #8698

j4james opened this issue Jan 3, 2021 · 11 comments · Fixed by #17510
Assignees
Labels
Impact-Correctness It be wrong. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty
Milestone

Comments

@j4james
Copy link
Collaborator

j4james commented Jan 3, 2021

Environment

Windows build number: Version 10.0.18363.1198
Windows Terminal version (if applicable): Version: 1.5.3242.0

Steps to reproduce

  1. Open a bash shell in Window Terminal.
  2. Execute the following command:
printf "\e[40m\e[2J\e]4;0;rgb:00/00/80\e\\"; sleep 1; printf "\e#8\e]4;0;rgb:FF/00/00\e\\"

Expected behavior

This should erase the screen with background color 0, but with the 0 color palette set to blue. It then pauses for a second, before filling the screen with the alignment test pattern (which uses default colors), and changes the 0 color palette to bright red.

Since the screen is filled with the default colors of the test pattern before the palette is changed to red, you should never see that red.

Actual behavior

There's a brief moment when the screen flashes bright red before the test pattern appears.

If you enable the "debug tap" you can see what's going wrong. The palette change is passed through to the conpty connection as soon as it's encountered, whereas the test pattern is written to the conhost buffer and only passed through to conpty in the next paint cycle. You thus end up having the operations arrive out of order.

I'm guessing it's timing related, so it may not happen all the time, but I'm definitely seeing it a lot.

@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 Jan 3, 2021
@DHowett
Copy link
Member

DHowett commented Jan 3, 2021

Oh yeah, this is absolutely a problem. ConPTY renders SGR and text during frame render, but passthrough is instant at time of dispatch failure. We've hit this in a number of places. @zadjii-msft knows more.

@j4james
Copy link
Collaborator Author

j4james commented Jan 4, 2021

I thought this had come up before, and I also thought there was an API somewhere that would force the frame to render before pass-through so we could trigger that on operations that needed it. I couldn't find anything like that, though, so maybe I imagined it.

@DHowett
Copy link
Member

DHowett commented Jan 4, 2021

Actually, yeah... hm. #4896

@j4james
Copy link
Collaborator Author

j4james commented Jan 4, 2021

OK, that's probably what I was thinking of, but that doesn't solve this problem. The content being flushed there is just the VT engine's output buffer. That includes the pass-through content that was just written, but it won't include any frame changes that haven't been painted yet. We need a paint to occur before the pass-through content is even added to that buffer. I think.

@DHowett
Copy link
Member

DHowett commented Jan 28, 2021

I'm not sure what to do with this one, but I am going to mark it up as a correctness issue in conpty and put it on the backlog. There will always be cases of this with our approach (especially when the control sequences in question apply to spans of text, ugh), and we need to revisit our approach if we want to get 100% of them.

Full passthrough for ENABLE_VIRTUAL_TERMINAL_PROCESSING apps with a low-fidelity text buffer would serve a great number of use cases. Perhaps this is worth factoring into that design.

@DHowett DHowett added Impact-Correctness It be wrong. Issue-Bug It either shouldn't be doing this or needs an investigation. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-3 A description (P3) Product-Conpty For console issues specifically related to conpty and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jan 28, 2021
@DHowett DHowett added this to the Console Backlog milestone Jan 28, 2021
@zadjii-msft zadjii-msft modified the milestones: Console Backlog, Backlog Jan 4, 2022
@zadjii-msft zadjii-msft removed the Needs-Tag-Fix Doesn't match tag requirements label Mar 23, 2022
@zadjii-msft
Copy link
Member

Some of the work in #12561 might help here. In that PR, I've got a helper that the alt buffer handling can use to manually flush the current frame and start buffering a new one. We may want to do that always when conpty is flushing through to the terminal side.

(we'll probably want to also do the thing where we don't actually WriteFile on the flush, for perf reasons. This would certainly exacerbate that hot path)

@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Mar 23, 2022
@zadjii-msft
Copy link
Member

zadjii-msft commented Jun 23, 2022

Discussed more with xterm.js folks - we should definitely fix this. Should be as trivial as doing the frame flush, then the sequence flush.

See also microsoft/vscode#151143

@zadjii-msft zadjii-msft modified the milestones: Backlog, Terminal v1.16 Jun 23, 2022
@zadjii-msft zadjii-msft removed the Priority-3 A description (P3) label Jun 23, 2022
@zadjii-msft
Copy link
Member

I don't know why this wouldn't work

there were lots of reasons

@zadjii-msft
Copy link
Member

zadjii-msft commented Apr 25, 2023

A note regarding #14677

That PR doesn't fix this. I that's because the _stateMachine->FlushToTerminal() call is still sticking the unknown sequence at the start of the frame, instead of at the end. The diagrams in #14677 (comment) are helpful. We still need something like #13462 for "paint the current frame, then append this sequence", or.... just... insert this sequence as something to add at the end of the current frame (and start the frame now). Hmm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Impact-Correctness It be wrong. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty
Projects
None yet
3 participants