terminal icon indicating copy to clipboard operation
terminal copied to clipboard

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

Open j4james opened this issue 4 years ago • 10 comments

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.

j4james avatar Jan 03 '21 23:01 j4james

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.

DHowett avatar Jan 03 '21 23:01 DHowett

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.

j4james avatar Jan 04 '21 00:01 j4james

Actually, yeah... hm. https://github.com/microsoft/terminal/pull/4896

DHowett avatar Jan 04 '21 00:01 DHowett

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.

j4james avatar Jan 04 '21 00:01 j4james

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 avatar Jan 28 '21 02:01 DHowett

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)

zadjii-msft avatar Mar 23 '22 15:03 zadjii-msft

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 https://github.com/microsoft/vscode/issues/151143

zadjii-msft avatar Jun 23 '22 15:06 zadjii-msft

Okay I've got a really dumb solution for this in e5fbc1bb1

import sys
import time

sys.stdout.write(f'foo\nbar\n\x1b]663;A\x1b\\baz\nqwer\n')
sys.stdout.flush()

image

I need to sit and actually think about this more, but I don't know why this wouldn't work

zadjii-msft avatar Jul 07 '22 18:07 zadjii-msft

That seems reasonable to me. As I understand it, that's the equivalent of what we're doing manually in a couple of places in AdaptDispatch when we call TriggerFlush before returning false like this:

https://github.com/microsoft/terminal/blob/c754f4d22da47d93ba6e1401496605eba45248d6/src/terminal/adapter/adaptDispatch.cpp#L2481-L2486

Assumedly those manual calls to TriggerFlush could now be dropped with this new approach.

j4james avatar Jul 07 '22 18:07 j4james

I don't know why this wouldn't work

there were lots of reasons

zadjii-msft avatar Jul 12 '22 22:07 zadjii-msft

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 https://github.com/microsoft/terminal/pull/14677#issuecomment-1499470956 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.~

zadjii-msft avatar Apr 25 '23 16:04 zadjii-msft