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

Use select() instead of mio polling in Unix EventSource #711

Closed
wants to merge 9 commits into from

Conversation

yyogo
Copy link
Contributor

@yyogo yyogo commented Sep 6, 2022

I've run into #500 (which is caused by tokio-rs/mio#1377) several times now, so here is a shot at reimplementing EventSource using select() instead of mio as discussed in the issue. I tested it a bit and it seems to work fine, but let me know if there's anything specific you want me to test.

I thought about using nix but decided against adding another dependency since using libc::select() is pretty straightforward.

Fixes #500, unblocks #407

@yyogo yyogo marked this pull request as ready for review September 6, 2022 16:28
// once more data is available to read.
io::ErrorKind::WouldBlock => break,
io::ErrorKind::Interrupted => continue,
_ => return Err(e),
Copy link
Contributor Author

@yyogo yyogo Sep 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch was missing, I believe it was a mistake, but this could be a behavior change

@yyogo
Copy link
Contributor Author

yyogo commented Sep 15, 2022

So unfortunately this solution is partial and will only work with fds < 1024. There is a workaround for Darwin, I'm working on implementing it , but it won't be pretty. And it would mean using poll/epoll() on Linux and only falling back to select on macOS, so I'm wondering if we're better off keeping mio and running an auxiliary thread that polls the tty.

@yyogo yyogo marked this pull request as draft September 15, 2022 15:47
@TimonPost
Copy link
Member

TimonPost commented Oct 9, 2022

Thanks for filing the PR and contributing to crossterm!

This solution is partial and will only work with fds < 1024

What does this imply?

I'm wondering if we're better off keeping mio and running an auxiliary thread that polls the tty

If the potential workaround is a dirty one, I would argue to stay with mio, and hope macOS supports tty in kqueue (which seems to be the issue). Tho if you argue the workaround outweighs the benefits of switching to poll, please provide the arguments why you think so, and then we can consider continuing the work here.

@yyogo
Copy link
Contributor Author

yyogo commented Oct 10, 2022

Hey @TimonPost , I'll try to summarize the situation as best I can.

As you said, the core issue is that kqueue does not support ttys. This is a long standing issue and I doubt it will be fixed any time soon, so I believe having a proper workaround is a good idea. poll and epoll are implemented in macos using kqueue, however select uses the older legacy implementation which does work with ttys. mio also uses kqueue and will not implement a workaround.

So the idea behind this PR was to switch both the Linux and macOS implementations to use select. However, it being a rather old syscall, it only support fds that are less than 1024 in value:

WARNING: select() can monitor only file descriptors numbers that
are less than FD_SETSIZE (1024)—an unreasonably low limit for
many modern applications—and this limitation will not change.
All modern applications should instead use poll(2) or epoll(7),
which do not suffer this limitation.
(from Linux's select(2) man page)

The implication of this limitation is that apps that open more than 1024 files will not be able to use the event stream after those files have been opened, since /dev/tty/ could potentially get a file descriptor larger than 1024. a hypothetical example of such a scenario would be an editor that is started with many filenames as arguments, and opens them before initializing the UI. Adding this limitation in macOS seems reasonable, since it won't break existing applications, however for linux it is a regression.

(In MacOS, there is a way to work around this limitation by compiling with different compile-time flags, which causes a different version of select to be linked, which can support arbitrary sized FD sets. However this is not implemented in libc or any other rust library that I could find.)

In short, we are left with the following options as I can tell:

  1. Use select universally and break fds >= 1024 in Linux (this is what rustty does)
  2. Implement poll/epoll for Linux and select for macOS
  3. A hybrid solution with mio (e.g spinning an extra thread on mac that polls using select and wakes mio)
  4. Do nothing

I'm willing to put in the work, just give me the go ahead on what you think is the best choice. I am leaning towards option 2, however there might be something I'm missing.

Cheers

@TimonPost
Copy link
Member

TimonPost commented Oct 10, 2022

I like solution 2, lets go for that.

  1. it uses the proper not deprecated API's on the platforms that can have problems with the deprecated one
  2. Using select on macOS while doing the workaround you speak of.
  3. Removing mio which is quite a large dependency (even tho we will have to reinvent the wheel a bit).

Perhaps, we could do a smooth transition by just creating a new event source as you did, and exposing it behind a feature flag. Just needs good manual testing on various platforms if we are going to do the polling our selves. Atm, I only have windows, but soon, I will possess a MacBook. Linux for me is only accessible through wsl 2. What platforms can you test on?

We just have to be sure our custom implementation works well enough and doesnt introduce more bugs then it tries to solve. I im a bit worried of knowing the nature of Linux terminal and differences per OS.

@conradludgate
Copy link

I'm trying to use crossterm in my tui app and this is a blocker for me, so I'm happy to help test. I have access to Linux and Mac environments

@yyogo
Copy link
Contributor Author

yyogo commented Oct 11, 2022

@TimonPost Sounds good to me, I'll get started then. I have all three platforms available to test on.

@conradludgate you can use this branch in the meantime, see helix-editor/helix#2111 (comment)

@conradludgate
Copy link

I already am in testing. It works great for me right now (using select). I won't merge it like this though

@sigmaSd
Copy link
Contributor

sigmaSd commented Nov 12, 2022

@yyogo thanks for your work, any update on this? is this still a draft or considered done?
This is currently probably the most requested feature probably in crossterm by unblocking #407

@sigmaSd
Copy link
Contributor

sigmaSd commented Nov 12, 2022

Also my personal opinion is to use select for macos without the hack.

I think as a fist cut that would be a nice step and the limitation seems theoretical. We can improve things when it gets tested in real world and people complain.

@yyogo
Copy link
Contributor Author

yyogo commented Nov 14, 2022

@sigmaSd this is still a draft, but I recently found filedescriptor's poll, which is used in termwiz and implements the polling logic I intended to. I already have a local branch with it and am currently doing some cleanups and preparing to open a new PR with it, expect it within a few days.

@yyogo
Copy link
Contributor Author

yyogo commented Dec 12, 2022

opened #735

@yyogo yyogo closed this Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

/dev/tty does not work on macOS with kqueue
4 participants