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

Cancellation on ReadAsync causes next ReadAsync to skip a byte (raw mode) #165

Open
scottbilas opened this issue Apr 28, 2024 · 9 comments
Labels
area: drivers Issues related to the terminal drivers. os: windows Issues that are specific to Windows (10, 11, etc). state: blocked Issues that are blocked on some other issue or work.
Milestone

Comments

@scottbilas
Copy link
Contributor

I'm unsure if this is a bug or I'm using the API wrong, but a canceled ReadAsync causes the next ReadAsync to ignore the next stdin byte, and then behaves normally after that.

To repro, run the below code. Type 'a', wait to allow the cancel, then type 'b' then 'c'. You will see a "c" for step 3, but it should show a "b".

Is there some state that I need to clear after a ReadAsync cancellation?

using static Vezel.Cathode.Terminal;

EnableRawMode();

var buf = new byte[1];

// read key press, works ok
await OutAsync("1: ");
await ReadAsync(buf);
await OutLineAsync((char)buf[0]);

// do a timeout cancel
try
{
    var cancel = new CancellationTokenSource();
    cancel.CancelAfter(100);

    await OutAsync("2: ");
    await ReadAsync(buf, cancel.Token);
    await OutLineAsync((char)buf[0]);
}
catch (OperationCanceledException)
{
    await OutLineAsync("cancel");
}

// this one requires two keypresses
await OutAsync("3: ");
await ReadAsync(buf);
await OutLineAsync((char)buf[0]);
@alexrp
Copy link
Member

alexrp commented Apr 28, 2024

What's the OS and terminal emulator?

@alexrp
Copy link
Member

alexrp commented Apr 28, 2024

To repro, run the below code. Type 'a', wait to allow the cancel, then type 'b' then 'c'.

AFAICS, the repro code will only allow two inputs due to the cancellation? I guess it's because I'm trying on Linux where it works properly. So I assume the bug is Windows-specific?

@scottbilas
Copy link
Contributor Author

This is on Windows in the Windows Terminal.

If I switch to WSL in the same terminal, it works as I'd expect. I also tested Mac, same. So it indeed looks like a bug just on Windows. I can work around it for now by setting up a separate task to read without a timeout into an additional pipe and putting the timeout on reading from that.

Separately, I noticed that on Mac/linux, OutLine only does \n but not also \r. I'd expect even on raw mode a higher level function like OutLine to do the "full newline", otherwise I'm not sure what the value of OutLine would be..behavior is surprising anyway. (Feels like the same sort of situation as the \n issue we worked through previously.)

@alexrp
Copy link
Member

alexrp commented Apr 29, 2024

This is on Windows in the Windows Terminal.

If I switch to WSL in the same terminal, it works as I'd expect. I also tested Mac, same. So it indeed looks like a bug just on Windows. I can work around it for now by setting up a separate task to read without a timeout into an additional pipe and putting the timeout on reading from that.

Ok, I'll investigate next time I boot into Windows.

Separately, I noticed that on Mac/linux, OutLine only does \n but not also \r. I'd expect even on raw mode a higher level function like OutLine to do the "full newline", otherwise I'm not sure what the value of OutLine would be..behavior is surprising anyway. (Feels like the same sort of situation as the \n issue we worked through previously.)

OutLine() and friends use Environment.NewLine, similar to the equivalent methods on System.Console. ReadLine() can't really be made workable in raw mode; it's considered valid only in cooked mode. So while OutLine() could be made to work by just always using \r\n, it would be inconsistent with ReadLine() and so be somewhat misleading, so I didn't. The right thing to do here is probably for these methods to throw in raw mode to clarify that they're higher-level cooked mode helpers.

@scottbilas
Copy link
Contributor Author

The right thing to do here is probably for these methods to throw in raw mode to clarify that they're higher-level cooked mode helpers.

Great idea 👍🏽

@alexrp alexrp self-assigned this Apr 29, 2024
@alexrp alexrp added this to the v1.0 milestone Apr 29, 2024
@alexrp alexrp added os: windows Issues that are specific to Windows (10, 11, etc). state: confirmed Bugs that have been confirmed and are reproducible. area: drivers Issues related to the terminal drivers. labels Apr 29, 2024
@alexrp
Copy link
Member

alexrp commented Apr 30, 2024

Ok, I can reproduce it. The behavior here is not encouraging.

I modified the repro to print the byte as an integer instead. If I press a, wait for cancellation, and then ø (Danish keyboard), I get:

dotnet run
1: 0x61
2: cancel
3: 0xb8

Whereas running Cathode's raw sample and pressing a + ø prints:

dotnet run --no-build
Entering raw mode.

0x61
0xc3
0xb8
...

So, something is indeed dropping a byte, even one that's part of a single code point, resulting in garbled input.

@alexrp
Copy link
Member

alexrp commented Apr 30, 2024

Bad news:

dotnet run
1: result=1, error=0, progress=1
0x61
2: result=0, error=995, progress=0
cancel
3: result=1, error=0, progress=1
0xb8

Those are the results of every ReadFile() call. The first two calls are as expected. The third call indicates that the 0xc3 byte never even reaches us. It's just lost.

I don't know if there's actually anything we can do about this. I'm tempted to say that we are, yet again, blocked by microsoft/terminal#12143...

@alexrp
Copy link
Member

alexrp commented Apr 30, 2024

It might be interesting to note that, if I modify the repro to not perform a third read, pressing ø after the program exits prints the ø properly in the shell. Whether that actually means anything useful, I'm not sure.

@alexrp
Copy link
Member

alexrp commented Apr 30, 2024

@scottbilas by the way, can I get you to upvote microsoft/terminal#12143? I need to somehow convince the team that it's worth spending time on fixing, and they seem to use upvotes a lot in their prioritization.

@alexrp alexrp added state: blocked Issues that are blocked on some other issue or work. and removed state: confirmed Bugs that have been confirmed and are reproducible. labels Apr 30, 2024
@alexrp alexrp modified the milestones: v1.0, Future Apr 30, 2024
@alexrp alexrp removed their assignment Apr 30, 2024
@alexrp alexrp removed the type: bug label Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: drivers Issues related to the terminal drivers. os: windows Issues that are specific to Windows (10, 11, etc). state: blocked Issues that are blocked on some other issue or work.
Development

No branches or pull requests

2 participants