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 flush unknown CSI sequences too early #15230

Closed
Mervap opened this issue Apr 24, 2023 · 9 comments
Closed

ConPTY flush unknown CSI sequences too early #15230

Mervap opened this issue Apr 24, 2023 · 9 comments
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation. Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing.

Comments

@Mervap
Copy link

Mervap commented Apr 24, 2023

Windows Terminal version

Not installed

Windows build number

10.0.19045.2846

Other Software

.NET 7.0 x64
PowerShell 7.3.4

Steps to reproduce

  1. Create ConPTY from dotnet (minimized version below, full working code here, based on samples/GUIConsole.ConPTY)
  2. Run powershell.exe with next ps1 script:
[Console]::Write("Begin")
[Console]::Write("`e[1q")
[Console]::Write("IN_BETWEEN_1")
[Console]::Write("`e[1Q")
[Console]::Write("`e[2q")
[Console]::Write("IN_BETWEEN_2")
[Console]::Write("`e[2Q")
[Console]::Write("`e[3q")
[Console]::Write("IN_BETWEEN_3")
[Console]::Write("`e[3Q")
[Console]::Write("Some long string at the end of output: aaaaaaaaaaaaaaa")
  1. Read charachters from PTY output
SafeFileHandle inputRead, inputWrite, outputRead, outputWrite;

CreatePseudoConsole(new COORD { X = 40, Y = 20 }, inputRead, outputWrite, 0, out IntPtr hPC);
ProcessFactory.Start("powershell.exe -File a.ps1", 0x00020016, hPC);

var reader = new StreamReader(new FileStream(outputRead, FileAccess.Read));

// Read output
var bytesRead = reader.ReadBlock(buf, 0, 140);
var output = new string(buf.Take(bytesRead).ToArray());
Console.Out.Write(output);

Expected Behavior

The entries in the output are in the same order, in which they were written

Begin←[1qIN_BETWEEN_1←[1Q←[2qIN_BETWEEN_2←[2Q←[3qIN_BETWEEN_3←[3Q

Actual Behavior

CSI sequences are printed before the text output

←[1q←[1Q←[2q←[2Q←[3q←[3QBeginIN_BETWEEN_1IN_BETWEEN_2IN_BETWEEN_3

I think it happens here
Don't you think that it's betteer to flush text buffer before flushing escape sequences?
And can you suggest some workaround? Can I flush text buffer manually from script?
Thanks

@Mervap Mervap added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Apr 24, 2023
@zadjii-msft
Copy link
Member

This looks a lot like what I was trying to fix over in #13710

@Mervap
Copy link
Author

Mervap commented Apr 24, 2023

@zadjii-msft seems #13710 relates to specific escape sequences, but my issue relates to any unknown (or not implemented) sequence

Let me please ask the question again: can I force buffer flushing somehow? To be sure that my output is synchronized. Probably it would be enough to solve my issue

@zadjii-msft
Copy link
Member

Ah, sorry, I linked the issue but not the PR. #14677 should fix this by flushing the frame when we encounter an unexpected sequence, then write the sequence, then start a new frame and continue. I just need to do the diligence on my part to re-check out that branch and double check your repro is fixed too.

There was also another thread that felt similar. #13462? Hey maybe I can close that out, since #14677 feels better to me

@j4james
Copy link
Collaborator

j4james commented Apr 25, 2023

FYI, I've tested PR #14677 with my test case from issue #8698, and it hasn't done anything to fix the order of operations there. The only thing I can see different when looking at the debug tap is that the DECTCEM (cursor show/hide) sequence appears in a slightly different position.

That said, from a visual standpoint it seems to be an improvement (at least for that particular test case), since I'm not seeing the red flash anymore. I'm not sure whether that's just a matter of lucky timing though. I suspect the position of the DECTCEM may be triggering a redraw in the renderer at a more appropriate time.

And with the test case in this issue, the ordering just seems random, both with and without PR #14677 applied. That suggests it's just dependent on when the render thread happens to trigger a flush.

@Mervap
Copy link
Author

Mervap commented Apr 25, 2023

You better know whether to close the issue or not 🙂
I just wanted to achieve 2 goals:

  1. Notify about the problem
  2. Find out if there is a workaround right now because I can't wait for the fix (and it's not a fact that it will be in <22000 at all)

Since you have not answered the 2nd question, I dare to assume that there is no workaround. Can you please confirm this?

@j4james
Copy link
Collaborator

j4james commented Apr 25, 2023

@Mervap I'm not personally aware of any workaround. It's also worth mentioning that passing through unknown sequences will potentially end up failing anyway, even if they were in the correct order. It depends on what your sequences are intended to do, but anything that modifies the buffer is at risk of being overwritten by conpty at a later point in time.

@Mervap
Copy link
Author

Mervap commented Apr 25, 2023

I understand that unknown sequences may behave really differently
In my case, it would be enough to force text buffer flushing before the escape sequence emitting (on pwsh script side)
Like

[Console]::Flush()
[Console]::Write("`e[1q")

@zadjii-msft
Copy link
Member

Okay yea, after caching this whole space back in, I'm gonna call this a /dupe of #8698. That's the more correct root issue here. Thanks again @j4james for the dilligence teasing them apart.

Right now, I'm not sure there's anything you could do to flush it manually on your side. maybe doing a `e[?1049h`e[?1049l to go into and out of the alt buffer, which would technically force a pair of frames, but also definitely horribly flicker. Yea definitely don't do that.


If you're building something on top of ConPTY, I might recommend upvoting #15065. Thats the thread we're using for "allow folks to use the most up-to-date conpty without relying on the OS version".

@microsoft-github-policy-service
Copy link
Contributor

Hi! We've identified this issue as a duplicate of another one that already exists on this Issue Tracker. This specific instance is being closed in favor of tracking the concern over on the referenced thread. Thanks for your report!

@microsoft-github-policy-service microsoft-github-policy-service bot added Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation. Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing.
Projects
None yet
Development

No branches or pull requests

3 participants