Repository: osa1/tiny (language: rust)

Issue:
Freezing on startup on ArchWSL
## Bug
When starting tiny on [ArchWSL](https://github.com/yuk7/ArchWSL) it will instantly freeze on startup no longer responding to any input.
I compiled it myself on the same system and there were no errors.

## Versions
tiny: Commit `4d3532`


Discussion / hints:
4d3532 is the version that works, right? What is the version that hangs? One of the binaries in the releases page?
`4d3532` is the version that hangs
i cannot reproduce this issue on version `v0.7.0` but for some reason i always get `Connection error: failed to lookup address information: Temporary failure in name resolution` which is really confusing because when i ping the exact same address it works just fine 
ive gotten that issue on `irc.esper.net` and also `irc.twitch.tv`. maybe my settings are incorrect?
I'm a bit busy this week but I'll try to install ArchWSL and reproduce the issue.

In the meantime, if you have the time, it would be good to know (1) if you can connect to those servers on Linux or another WSL (2) if you have the freeze issue on other WSLs.
i am able to reproduce both issues on ubuntu WSL
Do the issues exist in pre-built binaries in the release page? It'd be good if you could try with a libssl one and one without libssl.
both prebuilt relases (libssl and without libssl) run just fine.
i previously installed with `cargo install --git https://github.com/osa1/tiny.git --tag v0.7.0` and got the before mentioned issues. any ideas why?
>  i previously installed with cargo install --git https://github.com/osa1/tiny.git --tag v0.7.0 and got the before mentioned issues. any ideas why?

Hmm, this is weird. If 0.7.0 worked fine (pre-built binaries and when you build from source) but current master branch didn't then I'd suspect it could be https://github.com/osa1/tiny/commit/4a55e47d81f2dcd88326035b515d8670e8992e46, but if 0.7.0 doesn't work when you build from source then I have no idea.

So if I understand this correctly when you build from source (version doesn't matter) it doesn't work but pre-built binaries work. That's really weird.
when i build from the latest commit it freezes, when i build form 0.7.0 it doesn't connect and the prebuilt release works
This is introduced with 4a55e47. Here's what I tried:

- The last release works fine, both the pre-built binaries and when I build locally in Ubuntu WSL.
- Commits up to 4a55e47 also work fine when I build locally.
- With 4a55e47 the process hangs after about a second. It doesn't handle input events at all, but it seems to handle network events for about a second or so before freezing.

I wonder if this is a tokio bug. I should probably ask tokio devs first about any known issues on WSL.
Updating to tokio-0.3.4 doesn't fix the issue.
Example programs in term_input and termbox also hang. They don't use CPU when this happens.
echo_bot example in libtiny_tui works fine. "input" example in term_input only uses mio (no tokio) and it also doesn't work on WSL (works on Linux).
Interesting! Do you have any guesses on why I was experiencing network problems when building from source but not on the prebuilt version?
- I believe I tried with both the nightly and stable compiler.
- Which version of Ubuntu WSL are you using? I tried on version 20.04.
- Could I have installed a dependency such as OpenSSL incorrectly?
> Do you have any guesses on why I was experiencing network problems when building from source but not on the prebuilt version?

I have no idea..

> Which version of Ubuntu WSL are you using? I tried on version 20.04.

I'll check this evening when I can switch to Windows.

> Could I have installed a dependency such as OpenSSL incorrectly?

Possible, but note that I tried with rustls and observed the problem. So it may not be related to OpenSSL.

---

Update to my previous comment: the "input" example uses term_input. I was confused because I didn't see it imported, but we use it as qualified, e.g. `term_input::Input::new()`.

So it is still possible that the problem is in `term_input`.
It may not be related to the Ubuntu WSL version, as i got identical behaviour on ArchWSL
I fixed the issue locally (not pushed yet). Basically we need to set stdin fd to non-blocking mode otherwise `read()` blocks even in non-canonical mode. Interestingly, I had considered this case and did some research on this, and I found on Linux when in non-canonical mode you don't need to set stdin fd to non-blocking. I even documented it here: https://github.com/osa1/tiny/blob/997f6a808f88e4cf1072d16e5a9ac4d189a914b8/term_input/src/lib.rs#L8-L14

But apparently on WSL you need to set stdino to non-blocking mode even when you're in non-canonical mode. Not sure if this is a WSL bug or not ...
Interestingly, I did use tiny on WSL (ubuntu 18) without issues but since then I reset my system and now I'm experiencing the before mentioned issues, so it could be that an update to WSL caused this given I get the same behaviour on ArchWSL and UbuntuWSL.

Fix patch:
diff --git a/Cargo.lock b/Cargo.lock
index bf900ca4..48730a0d 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1041,6 +1041,7 @@ name = "term_input"
 version = "0.2.0"
 dependencies = [
  "libc",
+ "log",
  "mio",
  "nix",
  "term_input_macros",
diff --git a/term_input/Cargo.toml b/term_input/Cargo.toml
index 51ed4a58..6463cce0 100644
--- a/term_input/Cargo.toml
+++ b/term_input/Cargo.toml
@@ -9,6 +9,7 @@ edition = "2018"
 
 [dependencies]
 libc = "0.2"
+log = "0.4"
 mio = "0.7"
 nix = "0.19"
 term_input_macros = { path = "../term_input_macros" }
diff --git a/term_input/src/lib.rs b/term_input/src/lib.rs
index f6e0d09e..ef98edfd 100644
--- a/term_input/src/lib.rs
+++ b/term_input/src/lib.rs
@@ -4,14 +4,6 @@
 //!
 //! Keyboard events are read from `stdin`. We look for byte strings of key combinations that we
 //! care about. E.g. Alt-arrow keys, C-w etc.
-//!
-//! NOTE: Make sure to either set `stdin` to non-blocking mode or enable non-canonical input. For
-//! the latter, see:
-//!
-//! - https://www.gnu.org/software/libc/manual/html_node/Canonical-or-Not.html
-//! - https://www.gnu.org/software/libc/manual/html_node/Noncanonical-Input.html
-//!
-//! Also make sure to set `VMIN` and `VTIME` to zero in non-canonical mode.
 
 #[cfg(test)]
 mod tests;
@@ -22,11 +14,15 @@ use std::os::unix::io::RawFd;
 use std::pin::Pin;
 use std::task::{Context, Poll};
 
+use nix::fcntl::{fcntl, FcntlArg, OFlag};
 use tokio::io::unix::AsyncFd;
 use tokio::stream::Stream;
 
 use term_input_macros::byte_seq_parser;
 
+#[macro_use]
+extern crate log;
+
 ////////////////////////////////////////////////////////////////////////////////////////////////////
 // Public types
 ////////////////////////////////////////////////////////////////////////////////////////////////////
@@ -137,19 +133,41 @@ pub struct Input {
     buf: Vec<u8>,
 
     stdin: AsyncFd<RawFd>,
+
+    old_stdin_flags: Option<OFlag>,
+}
+
+impl Drop for Input {
+    fn drop(&mut self) {
+        if let Some(old_flags) = self.old_stdin_flags.take() {
+            match fcntl(libc::STDIN_FILENO, FcntlArg::F_SETFL(old_flags)) {
+                Err(err) => {
+                    error!("Unable to restore stdin flags: {:?}", err);
+                }
+                Ok(_) => {}
+            }
+        }
+    }
 }
 
 impl Input {
-    /// Create a new input handler. Requires a `tokio` reactor to be running. Make sure to either
-    /// set `stdin` to non-blocking mode or enable non-canonical input, otherwise the `Stream`
-    /// implementation will not work. If you are using this with `termbox` it does the latter so no
-    /// need to do anything. See module documentation for links on how to enable non-canonical
-    /// input.
+    /// Create an input handler. Requires a `tokio` reactor to be running. Sets `stdin` to
+    /// non-blocking mode. `stdin` flags are restored when the returned `Input` is dropped.
+    ///
+    /// Uses `stdin` so make sure you don't call this when there's another `Input` instance in the
+    /// process.
+    ///
+    /// Note that if you're using this with a terminal library like termbox you probably already
+    /// enable non-canonical input, in which case stdin doesn't need to be in non-blocking mode on
+    /// Linux, but on WSL we still need non-blocking mode, so this just sets stdin to non-blocking
+    /// mode always. See #269 for details.
     pub fn new() -> Input {
+        let old_stdin_flags = set_stdin_nonblocking();
         Input {
             evs: VecDeque::new(),
             buf: Vec::with_capacity(100),
             stdin: AsyncFd::new(libc::STDIN_FILENO).unwrap(),
+            old_stdin_flags,
         }
     }
 
@@ -398,3 +416,34 @@ pub fn read_stdin(buf: &mut Vec<u8>) -> Result<(), nix::Error> {
         }
     }
 }
+
+/// Set `stdin` to non-blocking mode. Returns old `stdin` if we were able to change the flags. Does
+/// not panic; errors are logged.
+fn set_stdin_nonblocking() -> Option<OFlag> {
+    let current_stdin_flags: OFlag = match fcntl(libc::STDIN_FILENO, FcntlArg::F_GETFL) {
+        Err(err) => {
+            error!("Unable to read stdin flags: {:?}", err);
+            return None;
+        }
+        Ok(flags) => match OFlag::from_bits(flags) {
+            None => {
+                error!("Unable to parse stdin flags: {:x?}", flags);
+                return None;
+            }
+            Some(flags) => flags,
+        },
+    };
+
+    let mut new_stdin_flags = current_stdin_flags;
+    new_stdin_flags.set(OFlag::O_NONBLOCK, true);
+
+    match fcntl(libc::STDIN_FILENO, FcntlArg::F_SETFL(new_stdin_flags)) {
+        Err(err) => {
+            // On Linux we don't really need to enable non-blocking mode so things should still
+            // work. On WSL things may or may not work.. see #269.
+            error!("Unable to set stdin flags: {:?}", err);
+            None
+        }
+        Ok(_) => Some(current_stdin_flags),
+    }
+}
diff --git a/termbox/src/lib.rs b/termbox/src/lib.rs
index 37cfd5fd..a54508d0 100644
--- a/termbox/src/lib.rs
+++ b/termbox/src/lib.rs
@@ -126,7 +126,9 @@ impl Termbox {
         new_term.c_cflag &= !(libc::CSIZE | libc::PARENB);
         new_term.c_cflag |= libc::CS8;
         // Enabled non-canonical mode above. Also set VMIN and VTIME = 0 so that `read(stdin)`
-        // won't block.
+        // won't block. References:
+        // - https://www.gnu.org/software/libc/manual/html_node/Canonical-or-Not.html
+        // - https://www.gnu.org/software/libc/manual/html_node/Noncanonical-Input.html
         new_term.c_cc[libc::VMIN] = 0;
         new_term.c_cc[libc::VTIME] = 0;
 


Test patch: