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 polling syscall directly to fix MacOS support #4

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

zeenix
Copy link
Contributor

@zeenix zeenix commented Mar 10, 2024

On MacOS, poll and kqueue are not supported for tty1. So let's ditch mio completely and directly call select ourselves.

On non-MacOS, we use poll to avoid limitation of select that is limited to FDs below 1024. If the process has a lot of open FDs, we could end up with an FD higher than 1023 and get an error.

This fixes MacOS support.

Fixes #3.


This replaces #2.

@Gelio
Copy link

Gelio commented Mar 10, 2024

Re-posting the comment from Canop/broot#854 (comment)

I checked out this PR and it looks like the problem is fixed for me on kitty

Kitty in tmux:

image

Kitty outside of tmux (says image protocol is supported, as expected):

image

Interestingly, the macOS Terminal app still has the extra characters:

image

On MacOS, `poll` and `kqueue` are not supported for tty[1]. So let's ditch
mio completely and directly call `select` ourselves.

On non-MacOS, we use `poll` to avoid limitation of `select` that is
limited to FDs below 1024. If the process has a lot of open FDs,
we could end up with an FD higher than 1023 and get an error.

This fixes MacOS support.

[1]: tokio-rs/mio#1377
@Canop
Copy link
Owner

Canop commented Mar 10, 2024

I find no problem, so I'm probably going to merge that.

No remaining doubt ?

@zeenix
Copy link
Contributor Author

zeenix commented Mar 10, 2024

I find no problem, so I'm probably going to merge that.

No remaining doubt ?

👍

@Canop Canop merged commit f9c5904 into Canop:main Mar 11, 2024
@zeenix zeenix deleted the use-select-on-mac branch March 11, 2024 12:03
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.

OS Error and answer leaking
3 participants