Replace mio polling with filedescriptor's poll() #735

yyogo · 2022-12-12T14:06:27Z

Dince mio doesn't handle ptys correctly macOS, use filedescriptor's poll() which falls back to select().

This PR replaces #711.

Fixes #500, unblocks #407.

Implementation notes

Since poll() doesn't support waking from signals or otherwise, I create two unix socket pairs, one which is written to on SIGWINCH and one which is used by the Waker, and poll on those as well as the tty fd.

src/event/source/unix.rs

YS-L · 2023-01-08T11:15:39Z

As a data point, I tested this with csvlens on multiple platforms (linux, macOS, Windows) and it works like a charm. It fixes the tty issue on macOS (YS-L/csvlens#20). Thank you for working on this!

TimonPost

Gonna give it a go on mac os now! Unamomento. Wonder if we should consider having a seperate EventSource asside from mio event source. Then it should be a matter of a feature toggle to enable or disable this behavior. This might be the safe way to transition away from mio. Do you think this might be worth the effort?

TimonPost · 2023-01-08T13:07:26Z

Also validated this works on macos

TimonPost

Let's get this merged (i'll wait a bit to see your reaction to the above question). Tho perhaps its easier to make a hard switch and also finally merge #407. Looks good! Thanks for the contribution.

yyogo · 2023-01-09T08:36:20Z

@TimonPost I agree a feature flag sounds like a safe way to introduce this change. Maybe the flag could be used to introduce #407 as well.

since mio doesn't handle ptys correctly macOS, use filedescriptor's poll() which falls back to select().

yyogo · 2023-01-09T09:26:20Z

I added the feature flag though it made the PR a bit messy. let me know if you want to merge it as-is, undo the feature gate, or something else

TimonPost · 2023-01-09T11:28:24Z

Yes this is good. WIll do a review asap

TimonPost · 2023-01-10T18:04:47Z

Cargo.toml

@@ -29,6 +29,7 @@ all-features = true
 default = ["bracketed-paste"]
 bracketed-paste = []
 event-stream = ["futures-core"]
+use-dev-tty = ["filedescriptor"]


dont we want to make a freature flag: mio and refer to the mio dependency, and make this enabled by default?

which would you like to be enabled by default?

I think default features are a bit of a pain to deal with since you can't disable them individually, so if we keep mio as the default by making it a default feature it would be troublesome for users to opt out, as well as possibly breaking. this seemed cleaner since it makes the new behavior opt in and the feature can be deprecated easily later. Also it's intended to include #407.

Good point. So then we temp can not opt out from mio but can opt in for the new feature.

TimonPost · 2023-01-10T18:05:14Z

src/event/source/unix.rs


-use mio::{unix::SourceFd, Events, Interest, Poll, Token};
-use signal_hook_mio::v0_8::Signals;
+#[cfg(not(feature = "use-dev-tty"))]


Then here we can use the mio feature flag

TimonPost · 2023-01-11T19:15:53Z

Great work @yyogo !

Copied ver batim from sigoden/aichat#264. ### Background - Identical bug: sigoden/aichat#257 - Request for docs: crossterm-rs/crossterm#849 - Similar bug: crossterm-rs/crossterm#644 (comment) - Source bug: crossterm-rs/crossterm#500 - Fix (enabled in this PR): crossterm-rs/crossterm#735 ### Error ``` │hread '<unnamed>' panicked at src/event.rs:46:45: │ │o events available: Custom { kind: Other, error: "Failed to initialize input reader" } │ ``` ### Backtrace In case of regression: ```diff ┌────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │key bfield shape │ │version 0.91.0 string │ │branch c string │ │commit_hash d string │ │build_os macos-x86_64 string │ │build_target x86_64-apple-darwin string │ │rust_version rustc 1.76.0 (07dca489a 2024-02-04) (Homebrew) string │ │cargo_version cargo 1.76.0 string │ │build_time 2024-03-05 20:28:40 +00:00 string │ │build_rust_channel release string │ │allocator mimalloc string │ │features dataframe, default, extra, sqlite, trash, which, zip string │ │installed_plugins nu_plugin_explore string │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ └────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ cell path: $.version NORMAL i to INSERT | hjkl to move around | p to peek | t to transpose | q to quit 0: 0x101a6ab65 - std::backtrace_rs::backtrace::libunwind::trace::he87ba3c236c7ad5f at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/../../backtrace/src/backtrace/libunwind.rs:104:5 1: 0x101a6ab65 - std::backtrace_rs::backtrace::trace_unsynchronized::h3ad5d899409e49ee at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5 2: 0x101a6ab65 - std::sys_common::backtrace::_print_fmt::hb1551f966d2dd86d at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/sys_common/backtrace.rs:68:5 3: 0x101a6ab65 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::hbd71adb7a72f4105 at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/sys_common/backtrace.rs:44:22 4: 0x101a8c993 - core::fmt::rt::Argument::fmt::h4224d647cce844bf at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/fmt/rt.rs:142:9 5: 0x101a8c993 - core::fmt::write::h30346430340bc336 at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/fmt/mod.rs:1120:17 6: 0x101a67a8e - std::io::Write::write_fmt::heb3d6316c565d5b1 at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/io/mod.rs:1810:15 7: 0x101a6a939 - std::sys_common::backtrace::_print::hc99e5bf521524ac2 at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/sys_common/backtrace.rs:47:5 8: 0x101a6a939 - std::sys_common::backtrace::print::h67e51ff2e3d5cfbd at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/sys_common/backtrace.rs:34:9 9: 0x101a6c575 - std::panicking::default_hook::{{closure}}::hc666c9a55318d1f1 10: 0x101a6c2ee - std::panicking::default_hook::hf980b1da49948523 at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panicking.rs:292:9 11: 0x100e53d54 - <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call::h09d0f3f719801ec9 at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/alloc/src/boxed.rs:2029:9 + 12: 0x100e4c81d - nu_plugin_explore::tui::Tui<B>::init::{{closure}}::h92cf9edd04f48f7a + at /Users/texas/Developer/git/nu_plugin_explore/src/tui.rs:45:13 13: 0x101a6cb75 - <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call::h3a63db2ca77cedb5 at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/alloc/src/boxed.rs:2029:9 14: 0x101a6cb75 - std::panicking::rust_panic_with_hook::h683bce980186bbbe at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panicking.rs:783:13 15: 0x101a6c904 - std::panicking::begin_panic_handler::{{closure}}::ha6dbd11ba0ec8af1 at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panicking.rs:657:13 16: 0x101a6b059 - std::sys_common::backtrace::__rust_end_short_backtrace::h889430bddc786c98 at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/sys_common/backtrace.rs:171:18 17: 0x101a6c642 - rust_begin_unwind at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panicking.rs:645:5 18: 0x101ab3f75 - core::panicking::panic_fmt::hff768cef35397791 at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/panicking.rs:72:14 19: 0x101ab4535 - core::result::unwrap_failed::h951d84d71b0bada2 at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/result.rs:1649:5 20: 0x100e86f0f - core::result::Result<T,E>::expect::h46e49f62a7f5b691 at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/result.rs:1030:23 + 21: 0x100e39983 - nu_plugin_explore::event::EventHandler::new::{{closure}}::h6a786ba814d713f3 + at /Users/texas/Developer/git/nu_plugin_explore/src/event.rs:46:24 22: 0x100e6d68d - std::sys_common::backtrace::__rust_begin_short_backtrace::h0d557fbf2c6545a9 at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/sys_common/backtrace.rs:155:18 23: 0x100e4f670 - std::thread::Builder::spawn_unchecked_::{{closure}}::{{closure}}::h49806568e55dc3ef at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/thread/mod.rs:529:17 24: 0x100e39260 - <core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once::ha7914519392245f9 at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/panic/unwind_safe.rs:272:9 25: 0x100e3dee0 - std::panicking::try::do_call::h77e6e80f923ba765 at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panicking.rs:552:40 26: 0x100e3df7d - ___rust_try 27: 0x100e3de59 - std::panicking::try::hb72a1d854f6956b8 at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panicking.rs:516:19 28: 0x100e4f4ad - std::panic::catch_unwind::h35d42a1f268a9228 at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panic.rs:142:14 29: 0x100e4f4ad - std::thread::Builder::spawn_unchecked_::{{closure}}::hd42e13ba971d3218 at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/thread/mod.rs:528:30 30: 0x100e74451 - core::ops::function::FnOnce::call_once{{vtable.shim}}::h6b49c774539034b2 at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/ops/function.rs:250:5 31: 0x101a712e9 - <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once::hef77fdfabbdc0490 at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/alloc/src/boxed.rs:2015:9 32: 0x101a712e9 - <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once::hd4d34ecf6438f9ac at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/alloc/src/boxed.rs:2015:9 33: 0x101a712e9 - std::sys::unix::thread::Thread::new::thread_start::hcdd70219a480b010 at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/sys/unix/thread.rs:108:17 34: 0x7ff81213918b - __pthread_start ```

yyogo requested a review from TimonPost as a code owner December 12, 2022 14:06

yyogo mentioned this pull request Dec 12, 2022

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

Closed

ellie mentioned this pull request Dec 18, 2022

crossterm support atuinsh/atuin#331

Merged

2 tasks

YS-L reviewed Dec 24, 2022

View reviewed changes

src/event/source/unix.rs Outdated Show resolved Hide resolved

the-mikedavis mentioned this pull request Dec 31, 2022

edit-command-line bug helix-editor/helix#5350

Closed

TimonPost reviewed Jan 8, 2023

View reviewed changes

TimonPost approved these changes Jan 8, 2023

View reviewed changes

yyogo added 3 commits January 9, 2023 11:19

replace mio polling with filedescriptor

e9fa5db

since mio doesn't handle ptys correctly macOS, use filedescriptor's poll() which falls back to select().

fix incorrect check of poll output

a1373d7

add use-dev-tty feature flag

be80323

yyogo force-pushed the use-filedescriptor branch from 8412934 to be80323 Compare January 9, 2023 09:23

yyogo mentioned this pull request Jan 9, 2023

Fix piping to Helix on macOS helix-editor/helix#5468

Merged

fix clippy warnings

6b390dc

TimonPost reviewed Jan 10, 2023

View reviewed changes

TimonPost approved these changes Jan 11, 2023

View reviewed changes

TimonPost merged commit 05229b7 into crossterm-rs:master Jan 11, 2023

This was referenced Jan 11, 2023

Changed STDIN to /dev/tty #407

Closed

Use dev/tty everywere #743

Draft

Expose Mio internals in public API #441

Open

openclosure mentioned this pull request Feb 23, 2023

Reevaluate the ability to pipe from stdin to buffer on MacOS helix-editor/helix#6089

Closed

yyogo mentioned this pull request Jul 12, 2023

Add cargo feature that enables piping on macos helix-editor/helix#7607

Closed

texastoland mentioned this pull request Mar 8, 2024

Fix panic on Mac from crossterm::event::poll amtoine/nu_plugin_explore#38

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace mio polling with filedescriptor's poll() #735

Replace mio polling with filedescriptor's poll() #735

yyogo commented Dec 12, 2022 •

edited

Loading

YS-L commented Jan 8, 2023

TimonPost left a comment

TimonPost commented Jan 8, 2023

TimonPost left a comment •

edited

Loading

yyogo commented Jan 9, 2023

yyogo commented Jan 9, 2023

TimonPost commented Jan 9, 2023

TimonPost Jan 10, 2023

yyogo Jan 11, 2023

TimonPost Jan 11, 2023

TimonPost Jan 10, 2023

TimonPost commented Jan 11, 2023

Replace mio polling with filedescriptor's poll() #735

Replace mio polling with filedescriptor's poll() #735

Conversation

yyogo commented Dec 12, 2022 • edited Loading

Implementation notes

YS-L commented Jan 8, 2023

TimonPost left a comment

Choose a reason for hiding this comment

TimonPost commented Jan 8, 2023

TimonPost left a comment • edited Loading

Choose a reason for hiding this comment

yyogo commented Jan 9, 2023

yyogo commented Jan 9, 2023

TimonPost commented Jan 9, 2023

TimonPost Jan 10, 2023

Choose a reason for hiding this comment

yyogo Jan 11, 2023

Choose a reason for hiding this comment

TimonPost Jan 11, 2023

Choose a reason for hiding this comment

TimonPost Jan 10, 2023

Choose a reason for hiding this comment

TimonPost commented Jan 11, 2023

yyogo commented Dec 12, 2022 •

edited

Loading

TimonPost left a comment •

edited

Loading