crossterm icon indicating copy to clipboard operation
crossterm copied to clipboard

/dev/tty does not work on macOS with kqueue

Open acidghost opened this issue 5 years ago • 7 comments

Describe the bug As described in tokio-rs/mio#1377, kqueue does not play well with /dev/tty. This is a problem for crossterm (as in #407), being unable to work properly.

To Reproduce See reproducer in tokio-rs/mio#1377.

Expected behavior Do not fail to poll on /dev/tty.

OS MacOs.

Terminal/Console Any.

Possible solutions The general solution is to use select instead of kqueue or epoll (poll should not work either). Changing Mio to support a select-based Poll requires major refactoring that is probably never going to happen. Given that mio::Poll is used in crossterm only (AFAIK) to poll SIGWINCH and /dev/tty, I suggest dropping the dependency on Mio and implement a select-based polling system with libc or nix.

Two major problems:

  • how to poll from fd and signal on non-linux (w/o signalfd);
  • how does it play with the waker system?

I'd be open to make a PR. Any suggestion?

acidghost avatar Oct 23 '20 08:10 acidghost

To handle signals we could use the signal_hook crate with pipes and select on /dev/tty and the read end of the pipe.

I've never used Rust asynchronous programming features (i.e. async/await) so I'm a bit lost for the event-stream feature.

acidghost avatar Oct 23 '20 09:10 acidghost

Here is some relevant code that uses this approach:

https://github.com/wez/wezterm/blob/f12df0be9b10491bdfdead3c4fc57918b208068d/termwiz/src/terminal/unix.rs#L390-L479

https://github.com/wez/wezterm/blob/f12df0be9b10491bdfdead3c4fc57918b208068d/filedescriptor/src/unix.rs#L421-L476

acidghost avatar Oct 23 '20 09:10 acidghost

That is sad to hear :(

There are 3 things at stake when speaking about rewrites:

  • Crossterm uses the signal crate with the mio extension for terminal size.
  • Crossterm uses MIO for polling event readiness on unix systems.
  • Crossterm has a Waker type which throws an event and wakes up a pending poll operation from MIO.

Also as for async input system, only the Waker and EventSource needs to function correctly. All other async logic is not of importance in this issue (as far I know).

There are some options here. First, given that this use case of /dev/tty only doesnt work on macos we could limit this feature and make macos use STDOUT by default. Which might bring some sad faces along.

Secondly, rewrite/extend the input system. The area of code I'm talking about can be found here

It 'could' be easy to replace the system. Since the logic responsible for all input/eventing logic is hidden behind a trait that is currently implemented for windows and unix. We can create a new implementation called MacOsEventSource for example. If this implementation works well, according to that which is expected behavior, it could be added here with a cfg(macos) macro.

Steps to create a new input system

  • Create a new EventSource like MacOsEventSource
  • Implement the try_read functionality, this function must be able to:
    • Wait for input for a certain duration
    • Return Some (TerminalResizeEvent, Input) or None if the duration elapsed.
    • Being able to wake up the read
  • Add the newly created EventSource to the InternalEventReader` here. This should make crossterm use a select based approach and therefore fixing all problems for macos.

This select based EventSource could serve as the default implementation for all Unix systems or we can limit it to MacOs (as spoken about above).

As far select goes, I am not that familiar with it and cant estimate how difficult it is to make it work within the EventSource trait.

TimonPost avatar Oct 23 '20 12:10 TimonPost

There are 3 things at stake when speaking about rewrites:

* Crossterm uses the signal crate with the mio extension for [terminal size](https://github.com/crossterm-rs/crossterm/blob/master/src/event/source/unix.rs#L129).

* Crossterm uses MIO for polling event readiness on unix systems.

* Crossterm has a `Waker` type which throws an event and wakes up a pending `poll` operation from MIO.

All of the above can be done w/o Mio, with just the signal_hook crate, select syscall and pipes.

There are some options here. First, given that this use case of /dev/tty only doesnt work on macos we could limit this feature and make macos use STDOUT by default. Which might bring some sad faces along.

I'm guessing you meant STDIN. The problem there, which is one I already faced while using crossterm, is that a downstream crate may want to read, e.g., a file from standard input.

This select based EventSource could serve as the default implementation for all Unix systems or we can limit it to MacOs (as spoken about above).

Indeed, an event source using select would work just fine on all UNIX systems: it's not used for polling from 100s of FDs and is the most supported syscall for polling (since the '80s). I'd be more keen to drop the dependency on Mio and rewrite the UnixEventSource.

Which of the two options would you prefer?

Also, I don't see tests around. How is this crate tested?

acidghost avatar Oct 23 '20 15:10 acidghost

Testing is mainly done with hand :). Just press some keys and see if it works. It is very hard to write tests for the input module since terminals differ from key combinations they support and we need to physically interact with the STDOUT/STDIN/ or /dev/tty. Which is not available in a CI environment. So its tricky.

If we can drop the MIO and are able to implement the same thing with 'select' I'm all the way in. It would make crossterm way smaller. So that's cool. Anyhow, as far I can tell it is quite easy to create a custom EventSource and create an implementation. If that's done we can test it and if it works we can drop it and switch to it completely. The async part should not be a deal as long the EventSource does its job well as described.

TimonPost avatar Oct 23 '20 16:10 TimonPost

Great, I'll work on a PR.

Do you have plans to merge #407 or shall I open a new PR?

That would just use tty_fd instead of STDIN in crossterm::terminal. E.g. here: https://github.com/crossterm-rs/crossterm/blob/4b1c8579142da89d0189f85aefe457d643c71491/src/terminal/sys/unix.rs#L122

acidghost avatar Oct 23 '20 16:10 acidghost

Nice!

This branch is waiting for the mac problem to be fixed, though that's what you going to do now. Thus I think closing the branch is better, it contains lots of debug code, and the important changes are not that big of a change. We should use the tty_fd function. And this function should first try /dev/tty but if not possible it should try stdin at the following places:

  • https://github.com/crossterm-rs/crossterm/blob/4b1c8579142da89d0189f85aefe457d643c71491/src/terminal/sys/unix.rs#L122
  • https://github.com/crossterm-rs/crossterm/blob/4b1c8579142da89d0189f85aefe457d643c71491/src/terminal/sys/unix.rs#L128

TimonPost avatar Oct 24 '20 08:10 TimonPost