terminal
terminal copied to clipboard
Conpty "pass-through" doesn't maintain order of operations
Environment
Windows build number: Version 10.0.18363.1198 Windows Terminal version (if applicable): Version: 1.5.3242.0
Steps to reproduce
- Open a bash shell in Window Terminal.
- 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.
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.
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.
Actually, yeah... hm. https://github.com/microsoft/terminal/pull/4896
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.
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.
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)
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
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()

I need to sit and actually think about this more, but I don't know why this wouldn't work
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.
I don't know why this wouldn't work
there were lots of reasons
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.~