crossterm-rs / crossterm

Cross platform terminal library rust
MIT License
3.23k stars 276 forks source link

Ncurses cannot receive resize events after crossterm read #447

Open samtay opened 4 years ago

samtay commented 4 years ago

Describe the bug Ncurses cannot receive window resize events after using crossterm::event::read.

To Reproduce Using cursive to demonstrate

crossterm::event::read().unwrap(); // blocks and continues after any event, e.g. key press
let mut siv = cursive::ncurses();
siv.add_layer(cursive::views::Dialog::text("foo"));
siv.run();

You'll notice that it doesn't auto resize to keep the dialog centered, because ncurses can't detect window resizing. If you remove the crossterm call, then it works just fine.

Expected behavior Expect ncurses to be able to operate as normal after using crossterm.

OS At least Arch Linux.

Terminal/Console At least kitty and terminator.

Context See the chat in the discord server. User MrCool seemed to be tracking down a cause.

sigmaSd commented 4 years ago

The issue is here https://github.com/crossterm-rs/crossterm/blob/master/src/event/source/unix.rs#L54 Signals struct needs to be dropped to un-register the signals https://docs.rs/signal-hook/0.1.16/src/signal_hook/iterator.rs.html#98 But even after adding the dropping the issue persists.

strace attests that the un-registering happens: close syscall is called on the fd used with sendto syscall and sendto is never called again after. currently close is never called and sendto syscall persist with each sigwinch indicating that the signal is still registered

I think:

1- crossterm needs to unregister this signals to free all resources, Since there is no crossterm struct or guard I think the only way is to add a fn clean() to the API that does this

2 - this issue might be in part to ncurses / singal_hook

code to reproduce:

use signal_hook::iterator::Signals;
fn main() {
    let mut signals = Signals::new(&[signal_hook::SIGWINCH]).unwrap();
    signals.close();
    drop(signals);

    ncurses::initscr();
    loop {
             if ncureses::getch() == 410 { // 410 is resize event
                       panic!("this wont happen") 
            }        
        }
}
sigmaSd commented 4 years ago

Based on https://github.com/vorner/signal-hook/issues/42#issuecomment-661853656 this is probably ncurses fault. I think a pragmatic solution might be to re-export signal_hook_cleanup::cleanup_signal and the signals we use like SIGWINCH or just add a note about this interaction in the documentation.