Make read sync block for windows systems

[matprec]

The documentation of TerminalInput states for TerminalInput::read_sync:

Readings will be blocking calls.

On windows this will resolve in call to ReadConsoleInput and according to MSDN it blocks.

The function does not return until at least one input record has been read.

But there is a problematic fast return, causing ReadConsoleInput not to be called.

https://github.com/TimonPost/crossterm/blob/8223cc9e4a5e35e15a0287bd9d8aa53dc84d1c34/crossterm_winapi/src/console.rs#L170-L173

Also some events return None, even though further events can be received:

https://github.com/TimonPost/crossterm/blob/8223cc9e4a5e35e15a0287bd9d8aa53dc84d1c34/crossterm_input/src/input/windows_input.rs#L230-L233

While this is valid behaviour for iterators, this causes early returns when iterating with for or while. This should be either changed or documented.

[matprec]

My suggestions would be to default to length 1, to actually block.

[imdaveho]

@MSleepyPanda could you create a toy example? You're probably right, if SyncReader blocks anyway, then we probably don't want to fast return too soon or at all really...

[matprec]

Sure

This immediately exits on my machine (println to prevent llvm from optimizing out).

fn main() {
    let input = crossterm::input();

    for _ in input.read_sync() {
        println!("Hello World!");
    }
}

Hacky workaround:

fn main() {
    let input = crossterm::input();

    loop {
        for _ in input.read_sync() {
            println!("Hello World!");
        }
    }
}

This busy loops (1 core @ 100%) and leaks memory in the console subsytem (conhost.exe)

I've confirmed locally that a) patch sections in cargo are a mess and b) defaulting to length 1 fixes this.

[TimonPost]

Hey, thanks for the issue, I'll see what I can do here and what valid solutions would be I take your suggestion into account.

[TimonPost]

Yea so basicly TUI had the same problem with this and should be fixed. Windows returns directly if there is nothing to read and Linux will wait for input before returning.

[matprec]

Iirc this can be resolved by solely removing the length check, as it's not even used further down in the code, at least in the master branch.

Backporting it to 0.9 might also make sense.

I'm on vacation over the next two weeks and thus not able to create a PR in the meantime, but if its still unresolved by then i'll give it a shot. If someone is interested, feel free to take it :)

[joshhansen]

Just ran into this issue on the current master branch. I've been digging into it (stymied a bit by the difficulty of debugging Rust on Windows, but I'm doing my best) and wanted to share what I've found.

Simply removing the length check and early return in read_single_input_event does not resolve the issue. Even with this change, @MSleepyPanda's toy example continues to exit without going through a single loop of the iteration.

read_single_event (called by SyncReader::next) doesn't block either when called on its own so the problem lives there somewhere. The console.read_single_input_event() call in read_single_event returns a KeyEvent even if no key is pressed---I don't know the Windows API so I don't know whether this is normal or not, but it seems weird. When this pseudo-keypress comes in, read_single_event calls handle_key_event which attempts to parse the pseudo-keypress but fails to do so, and returns an Ok(None) which is what actually appears to lead the iterator to terminate early.

I haven't checked it out yet but I would speculate that there's something wrong with how the FFI boundary is getting crossed, leading to the appearance of an event where none exists, specifically pub unsafe fn KeyEvent(&self) -> &KEY_EVENT_RECORD on winapi::um::wincon::INPUT_RECORD_Event, called by read_single_event. But the code gets very macro-y right about there and I haven't pursued it deeper than that.

I thought I'd share my progress thus far in case somebody can take it farther.

[matprec]

@joshhansen You have to keep in mind that the first example may also early return because some events are not handled

So you still have to use a loop, but it shouldn't pin a core to 100% anymore.

[TimonPost]

@MSleepyPanda can you checkout out crossterm-rs/crossterm-winapi#2 because it uses a loop as solutions. The .net framework seems to do it like that as well. I am not sure if there are any other ways on windows to block read for input.

[joshhansen]

@MSleepyPanda Ah, I see. Is there any reason not to just ignore the unhandled events instead of terminating the iteration?

[matprec]

@TimonPost I'll have a look on wednesday :)

I had a quick look but i think this would still busy loop because you early return and don't give the windows api a chance to block the thread.

[matprec]

@joshhansen I think ignoring makes sense, but it wasn't straightforward within the current architecture iirc. I'm still on vacation, but i'll be back on wednesday.

[matprec]

@TimonPost Sorry for the delay, i just submitted my review crossterm-rs/crossterm-winapi#2 (review)

[joshhansen]

I'm still getting early exit from read_sync on Windows with 0.12.0. Did this make it into the 0.12.0 release?

[matprec]

Apparently not, the code in question is here

crossterm/src/input/input/windows.rs

Line 216 in 2815833

read_single_event().unwrap_or(None)

The current behaviour is documented here

crossterm/src/input/input/windows.rs

Lines 159 to 162 in 2815833

	/// `SyncReader` is an individual iterator and it doesn't use `None` to indicate that the iteration is
	/// finished. You can expect additional `Some(InputEvent)` after calling `next` even if you have already
	/// received `None`. Unfortunately, `None` means that an error occurred, but you're free to call `next`
	/// again. This behavior will be changed in the future to avoid errors consumption.

If you loop over reader.next() you should be fine for now.