Repository: osa1/tiny (language: rust)

Issue:
Panic in pinger.rs
I was editing a message using the text editor through the `C-x` hotkey. When I closed the editor I saw this message interleaved with the channel text (probably because it went to stderr). Despite the panic tiny did not actually crash. It displayed the normal ping timeout message and successfully reconnected afterwards.

> panicked at 'called `Result::unwrap()` on an `Err` value: Closed(SendPing)', libtiny_client/src/pinger.rs:40:9

https://github.com/osa1/tiny/blob/f8477c322a0eb464072542f75c50ecfeb587bbd0/libtiny_client/src/pinger.rs#L40


Discussion / hints:
Thanks for reporting. I think this is just a manifestation of a long-standing problem with the `C-x` implementation. It currently stops *all* tasks, that includes the tasks that drive connections, timer tasks for checking connectivity etc. If you spend long-enough time in your editor after `C-x` it causes all kinds of problems.

The solution should be to run the editor in a new thread, allowing background tasks to run. This may be as easy as wrapping the call with `tokio::task::spawn_blocking`. Only difficulty with this is that the TUI task needs to wait for the blocking task to return. 
I thought a little bit about how to best fix this isssue with `C-x`. Here's an idea:

- First of all `termbox` should support `present()` when suspended. It'd basically just update the internal buffer but do not write the buffer to the tty when suspended, until it's activated again.
  This is easy to implement as `termbox` already works without a tty (used in tests).

- Once we have that we'll be able to suspend the TUI while still being able to do updates. This will allow us to show `$EDITOR` in a new thread while all the connection tasks update the TUI.

- TUI is not `Send` or `Sync` so we won't be able to have a reference to it in the `$EDITOR` thread. Instead we will send an event to the TUI via the event channel. The problem here is that normally TUI only has the receiver end of this channel. So to be able to do it we'll also keep a sender in TUI, and clone and pass it to the `$EDITOR` thread. This will also require a new event type.

After this it should be possible to run `$EDITOR` for hours if we wanted.

Fix patch:
diff --git a/libtiny_tui/src/editor.rs b/libtiny_tui/src/editor.rs
index c7bf4bf6..613d0403 100644
--- a/libtiny_tui/src/editor.rs
+++ b/libtiny_tui/src/editor.rs
@@ -1,35 +1,53 @@
+//! This module implements some part of running $EDITOR to edit input. The other part is
+//! implemented at the top-level of the current crate (in `input_handler` task).
+//!
+//! Implementation is a little bit hacky: we need to give control of tty, stdout, and stdin to
+//! $EDITOR while still running the tokio event loop, to handle connection events. Here's how we
+//! implement this:
+//!
+//! - $EDITOR runs in a new thread to avoid blocking tokio event loop.
+//!
+//! - termbox implement a 'suspend' method that restores the terminal and stops using tty, until
+//!   'activate' is called. We can still use termbox methods as usual, but termbox doesn't really
+//!   draw anything. This way we don't need to stop rendering TUI when $EDITOR is running, we just
+//!   'suspend' termbox and keep running the TUI as usual in its own task.
+//!
+//! - We use a oneshot channel from the spawned $EDITOR thread to the TUI input handler task, to
+//!   send the result. When the channel is available (not `None`) it means $EDITOR is running and
+//!   we should not read stdin. So the stdin handler task just blocks on this channel when it is
+//!   available. This happens in `input_handler` task at the top-level of the crate.
+
 use termbox_simple::Termbox;
+use tokio::sync::oneshot;
 
 #[derive(Debug)]
-pub(crate) enum EditorError {
+pub(crate) enum Error {
     Io(::std::io::Error),
     Var(::std::env::VarError),
 }
 
-impl From<::std::io::Error> for EditorError {
-    fn from(err: ::std::io::Error) -> EditorError {
-        EditorError::Io(err)
+impl From<::std::io::Error> for Error {
+    fn from(err: ::std::io::Error) -> Error {
+        Error::Io(err)
     }
 }
 
-impl From<::std::env::VarError> for EditorError {
-    fn from(err: ::std::env::VarError) -> EditorError {
-        EditorError::Var(err)
+impl From<::std::env::VarError> for Error {
+    fn from(err: ::std::env::VarError) -> Error {
+        Error::Var(err)
     }
 }
 
-/// The user tried to paste the multi-line string passed as the argument. Run $EDITOR to edit a
-/// temporary file with the string as the contents. On exit, parse the final contents of the file
-/// (ignore comment lines), and send each line in the file as a message. Abort if any of the lines
-/// look like a command (e.g. `/msg ...`). I don't know what's the best way to handle commands in
-/// this context.
-///
-/// Ok(str) => final string to send
-/// Err(str) => err message to show
-///
-/// FIXME: Ideally this function should get a `Termbox` argument and return a new `Termbox` because
-/// we shutdown the current termbox instance and initialize it again after running $EDITOR.
-pub(crate) fn edit(tb: &mut Termbox, tf: String, str: &str) -> Result<Vec<String>, EditorError> {
+pub(crate) type Result<A> = ::std::result::Result<A, Error>;
+
+pub(crate) type ResultReceiver = oneshot::Receiver<Result<Vec<String>>>;
+
+pub(crate) fn run(
+    tb: &mut Termbox,
+    text_field_contents: &str,
+    pasted_text: &str,
+    rcv_editor_ret: &mut Option<ResultReceiver>,
+) -> Result<()> {
     use std::{
         io::{Read, Seek, SeekFrom, Write},
         process::Command,
@@ -45,38 +63,63 @@ pub(crate) fn edit(tb: &mut Termbox, tf: String, str: &str) -> Result<Vec<String
          # this file will be sent (ignoring these lines). Delete contents to abort the\n\
          # paste."
     )?;
-    write!(tmp_file, "{}", tf)?;
-    write!(tmp_file, "{}", str.replace('\r', "\n"))?;
+    write!(tmp_file, "{}", text_field_contents)?;
+    write!(tmp_file, "{}", pasted_text.replace('\r', "\n"))?;
+
+    let (snd_editor_ret, rcv_editor_ret_) = oneshot::channel();
+    *rcv_editor_ret = Some(rcv_editor_ret_);
 
+    // Will be activated by the input handler task after reading rcv_editor_ret
     tb.suspend();
-    let ret = Command::new(editor).arg(tmp_file.path()).status();
-    tb.activate();
 
-    let ret = ret?;
-    if !ret.success() {
-        return Ok(vec![]); // assume aborted
-    }
+    // At this point termbox is suspended (terminal settings restored), and the input handler task
+    // won't be reading term_input until it reads from rcv_editor_ret, so the editor has control
+    // over the tty, stdout, and stdin.
+    tokio::task::spawn_blocking(move || {
+        let ret = Command::new(editor).arg(tmp_file.path()).status();
+        let ret = match ret {
+            Err(io_err) => {
+                snd_editor_ret.send(Err(io_err.into())).unwrap();
+                return;
+            }
+            Ok(ret) => ret,
+        };
 
-    let mut tmp_file = tmp_file.into_file();
-    tmp_file.seek(SeekFrom::Start(0))?;
-
-    let mut file_contents = String::new();
-    tmp_file.read_to_string(&mut file_contents)?;
-
-    let mut filtered_lines = vec![];
-    for s in file_contents.lines() {
-        // Ignore if the char is '#'. To actually send a `#` add space.
-        // For empty lines, send " ".
-        let first_char = s.chars().next();
-        if first_char == Some('#') {
-            // skip this line
-            continue;
-        } else if s.is_empty() {
-            filtered_lines.push(" ".to_owned());
-        } else {
-            filtered_lines.push(s.to_owned());
+        if !ret.success() {
+            // Assume aborted
+            snd_editor_ret.send(Ok(vec![])).unwrap();
+            return;
         }
-    }
 
-    Ok(filtered_lines)
+        let mut tmp_file = tmp_file.into_file();
+        if let Err(io_err) = tmp_file.seek(SeekFrom::Start(0)) {
+            snd_editor_ret.send(Err(io_err.into())).unwrap();
+            return;
+        }
+
+        let mut file_contents = String::new();
+        if let Err(io_err) = tmp_file.read_to_string(&mut file_contents) {
+            snd_editor_ret.send(Err(io_err.into())).unwrap();
+            return;
+        }
+
+        let mut filtered_lines = vec![];
+        for s in file_contents.lines() {
+            // Ignore if the char is '#'. To actually send a `#` add space.
+            // For empty lines, send " ".
+            let first_char = s.chars().next();
+            if first_char == Some('#') {
+                // skip this line
+                continue;
+            } else if s.is_empty() {
+                filtered_lines.push(" ".to_owned());
+            } else {
+                filtered_lines.push(s.to_owned());
+            }
+        }
+
+        snd_editor_ret.send(Ok(filtered_lines)).unwrap();
+    });
+
+    Ok(())
 }
diff --git a/libtiny_tui/src/lib.rs b/libtiny_tui/src/lib.rs
index 010dda5c..4a54af68 100644
--- a/libtiny_tui/src/lib.rs
+++ b/libtiny_tui/src/lib.rs
@@ -103,14 +103,44 @@ async fn input_handler(
     mut snd_abort: mpsc::Sender<()>,
 ) {
     let mut input = Input::new();
-    while let Some(mb_ev) = input.next().await {
-        match mb_ev {
-            Err(io_err) => {
+
+    // See module documentation of `editor` for how editor stuff works
+    let mut rcv_editor_ret: Option<editor::ResultReceiver> = None;
+
+    loop {
+        if let Some(editor_ret) = rcv_editor_ret.take() {
+            // $EDITOR running, don't read stdin, wait for $EDITOR to finish
+            match editor_ret.await {
+                Err(recv_error) => {
+                    debug!("RecvError while waiting editor response: {:?}", recv_error);
+                }
+                Ok(editor_ret) => {
+                    if let Some((lines, from)) = tui.borrow_mut().handle_editor_result(editor_ret) {
+                        debug!("editor ret: {:?}", lines);
+                        snd_ev
+                            .try_send(Event::Lines {
+                                lines,
+                                source: from,
+                            })
+                            .unwrap();
+                    }
+                }
+            }
+
+            tui.borrow_mut().activate();
+            tui.borrow_mut().draw();
+        }
+
+        match input.next().await {
+            None => {
+                break;
+            }
+            Some(Err(io_err)) => {
                 debug!("term_input error: {:?}", io_err);
-                return;
+                break;
             }
-            Ok(ev) => {
-                let tui_ret = tui.borrow_mut().handle_input_event(ev);
+            Some(Ok(ev)) => {
+                let tui_ret = tui.borrow_mut().handle_input_event(ev, &mut rcv_editor_ret);
                 match tui_ret {
                     TUIRet::Abort => {
                         snd_ev.try_send(Event::Abort).unwrap();
@@ -135,17 +165,10 @@ async fn input_handler(
                                 .unwrap();
                         }
                     }
-                    TUIRet::Lines { lines, from } => {
-                        snd_ev
-                            .try_send(Event::Lines {
-                                lines,
-                                source: from,
-                            })
-                            .unwrap();
-                    }
                 }
             }
         }
+
         tui.borrow_mut().draw();
     }
 }
diff --git a/libtiny_tui/src/tui.rs b/libtiny_tui/src/tui.rs
index b7d226e4..9d0f2007 100644
--- a/libtiny_tui/src/tui.rs
+++ b/libtiny_tui/src/tui.rs
@@ -8,7 +8,7 @@ use std::str::SplitWhitespace;
 use time::Tm;
 
 use crate::config::{parse_config, Colors, Config, Style};
-use crate::editor::{edit, EditorError};
+use crate::editor;
 use crate::messaging::{MessagingUI, Timestamp};
 use crate::notifier::Notifier;
 use crate::statusline::{draw_statusline, statusline_visible};
@@ -33,12 +33,6 @@ pub(crate) enum TUIRet {
         msg: Vec<char>,
         from: MsgSource,
     },
-
-    /// A pasted string. Send directly.
-    Lines {
-        lines: Vec<String>,
-        from: MsgSource,
-    },
 }
 
 const LEFT_ARROW: char = '<';
@@ -85,6 +79,10 @@ impl TUI {
         &self.tb
     }
 
+    pub(crate) fn activate(&mut self) {
+        self.tb.activate()
+    }
+
     fn new_tb(config_path: Option<PathBuf>, tb: Termbox) -> TUI {
         // This is now done in reload_config() below
         // tb.set_clear_attributes(colors.clear.fg as u8, colors.clear.bg as u8);
@@ -438,20 +436,24 @@ impl TUI {
         self.fix_scroll_after_close();
     }
 
-    pub(crate) fn handle_input_event(&mut self, ev: Event) -> TUIRet {
+    pub(crate) fn handle_input_event(
+        &mut self,
+        ev: Event,
+        rcv_editor_ret: &mut Option<editor::ResultReceiver>,
+    ) -> TUIRet {
         match ev {
-            Event::Key(key) => self.keypressed(key),
+            Event::Key(key) => self.keypressed(key, rcv_editor_ret),
 
             Event::String(str) => {
                 // For some reason on my terminal newlines in text are
                 // translated to carriage returns when pasting so we check for
                 // both just to make sure
                 if str.contains('\n') || str.contains('\r') {
-                    return self.edit_input(&str);
+                    self.run_editor(&str, rcv_editor_ret);
                 } else {
                     // TODO this may be too slow for pasting long single lines
                     for ch in str.chars() {
-                        self.handle_input_event(Event::Key(Key::Char(ch)));
+                        self.handle_input_event(Event::Key(Key::Char(ch)), rcv_editor_ret);
                     }
                 }
                 TUIRet::KeyHandled
@@ -461,60 +463,76 @@ impl TUI {
         }
     }
 
-    /// Edit current input + `str` before sending.
-    fn edit_input(&mut self, str: &str) -> TUIRet {
-        let tab = &mut self.tabs[self.active_idx].widget;
-        let tf = tab.flush_input_field();
-        match edit(&mut self.tb, tf, &str) {
+    pub(crate) fn handle_editor_result(
+        &mut self,
+        editor_ret: editor::Result<Vec<String>>,
+    ) -> Option<(Vec<String>, MsgSource)> {
+        match editor_ret {
+            Err(err) => {
+                self.handle_editor_err(err);
+                None
+            }
             Ok(lines) => {
+                let tab = &mut self.tabs[self.active_idx].widget;
                 // If there's only one line just add it to the input field, do not send it
                 if lines.len() == 1 {
                     tab.set_input_field(&lines[0]);
-                    TUIRet::KeyHandled
+                    None
                 } else {
                     // Otherwise add the lines to text field history and send it
                     for line in &lines {
                         tab.add_input_field_history(line);
                     }
-                    TUIRet::Lines {
-                        lines,
-                        from: self.tabs[self.active_idx].src.clone(),
-                    }
+                    Some((lines, self.tabs[self.active_idx].src.clone()))
                 }
             }
-            Err(err) => {
-                use std::env::VarError;
-                match err {
-                    EditorError::Io(err) => {
-                        self.add_client_err_msg(
-                            &format!("Error while running $EDITOR: {:?}", err),
-                            &MsgTarget::CurrentTab,
-                        );
-                    }
-                    EditorError::Var(VarError::NotPresent) => {
-                        self.add_client_err_msg(
-                            "Can't paste multi-line string: \
+        }
+    }
+
+    /// Edit current input + `str` before sending.
+    fn run_editor(&mut self, str: &str, rcv_editor_ret: &mut Option<editor::ResultReceiver>) {
+        let tab = &mut self.tabs[self.active_idx].widget;
+        let tf = tab.flush_input_field();
+        match editor::run(&mut self.tb, &tf, &str, rcv_editor_ret) {
+            Ok(()) => {}
+            Err(err) => self.handle_editor_err(err),
+        }
+    }
+
+    fn handle_editor_err(&mut self, err: editor::Error) {
+        use std::env::VarError;
+        match err {
+            editor::Error::Io(err) => {
+                self.add_client_err_msg(
+                    &format!("Error while running $EDITOR: {:?}", err),
+                    &MsgTarget::CurrentTab,
+                );
+            }
+            editor::Error::Var(VarError::NotPresent) => {
+                self.add_client_err_msg(
+                    "Can't paste multi-line string: \
                              make sure your $EDITOR is set",
-                            &MsgTarget::CurrentTab,
-                        );
-                    }
-                    EditorError::Var(VarError::NotUnicode(_)) => {
-                        self.add_client_err_msg(
-                            "Can't paste multi-line string: \
+                    &MsgTarget::CurrentTab,
+                );
+            }
+            editor::Error::Var(VarError::NotUnicode(_)) => {
+                self.add_client_err_msg(
+                    "Can't paste multi-line string: \
                              can't parse $EDITOR (not unicode)",
-                            &MsgTarget::CurrentTab,
-                        );
-                    }
-                }
-                TUIRet::KeyHandled
+                    &MsgTarget::CurrentTab,
+                );
             }
         }
     }
 
-    fn keypressed(&mut self, key: Key) -> TUIRet {
+    fn keypressed(
+        &mut self,
+        key: Key,
+        rcv_editor_ret: &mut Option<editor::ResultReceiver>,
+    ) -> TUIRet {
         match self.tabs[self.active_idx].widget.keypressed(key) {
             WidgetRet::KeyHandled => TUIRet::KeyHandled,
-            WidgetRet::KeyIgnored => self.handle_keypress(key),
+            WidgetRet::KeyIgnored => self.handle_keypress(key, rcv_editor_ret),
             WidgetRet::Input(input) => TUIRet::Input {
                 msg: input,
                 from: self.tabs[self.active_idx].src.clone(),
@@ -524,7 +542,11 @@ impl TUI {
         }
     }
 
-    fn handle_keypress(&mut self, key: Key) -> TUIRet {
+    fn handle_keypress(
+        &mut self,
+        key: Key,
+        rcv_editor_ret: &mut Option<editor::ResultReceiver>,
+    ) -> TUIRet {
         match key {
             Key::Ctrl('n') => {
                 self.next_tab();
@@ -536,7 +558,10 @@ impl TUI {
                 TUIRet::KeyHandled
             }
 
-            Key::Ctrl('x') => self.edit_input(""),
+            Key::Ctrl('x') => {
+                self.run_editor("", rcv_editor_ret);
+                TUIRet::KeyHandled
+            }
 
             Key::AltChar(c) => match c.to_digit(10) {
                 Some(i) => {
diff --git a/termbox/src/lib.rs b/termbox/src/lib.rs
index 18e3177d..a6c0b77b 100644
--- a/termbox/src/lib.rs
+++ b/termbox/src/lib.rs
@@ -18,6 +18,8 @@ pub const TB_UNDERLINE: u16 = 0x0200;
 pub struct Termbox {
     // Not available in test instances
     tty: Option<File>,
+    // Used to save `tty` on `suspend` and restore on `activate`
+    old_tty: Option<File>,
     old_term: libc::termios,
     term_width: u16,
     term_height: u16,
@@ -137,6 +139,7 @@ impl Termbox {
         front_buffer.clear(clear_fg, clear_bg);
         let mut termbox = Termbox {
             tty: Some(tty),
+            old_tty: None,
             old_term,
             term_width,
             term_height,
@@ -162,6 +165,7 @@ impl Termbox {
     pub fn init_test(w: u16, h: u16) -> Termbox {
         Termbox {
             tty: None,
+            old_tty: None,
             old_term: unsafe { std::mem::zeroed() },
             term_width: w,
             term_height: h,
@@ -191,6 +195,7 @@ impl Termbox {
     // HACKY
     pub fn suspend(&mut self) {
         self.flip_terms();
+        self.old_tty = self.tty.take();
 
         self.output_buffer
             .extend_from_slice(termion::cursor::Show.as_ref());
@@ -207,6 +212,7 @@ impl Termbox {
     // HACKY
     pub fn activate(&mut self) {
         self.flip_terms();
+        self.tty = self.old_tty.take();
 
         // T_ENTER_CA for xterm
         if let Some(ref mut tty) = self.tty {


Test patch:
diff --git a/libtiny_tui/src/tests/mod.rs b/libtiny_tui/src/tests/mod.rs
index aa301055..9fb6485b 100644
--- a/libtiny_tui/src/tests/mod.rs
+++ b/libtiny_tui/src/tests/mod.rs
@@ -70,7 +70,7 @@ fn expect_screen(screen: &str, tui: &TUI, w: u16, h: u16, caller: &'static Locat
 
 fn enter_string(tui: &mut TUI, s: &str) {
     for c in s.chars() {
-        tui.handle_input_event(Event::Key(Key::Char(c)));
+        tui.handle_input_event(Event::Key(Key::Char(c)), &mut None);
     }
 }
 
@@ -240,7 +240,7 @@ fn ctrl_w() {
          |< irc.server_1.org #chan      |";
     expect_screen(screen, &tui, 30, 3, Location::caller());
 
-    tui.handle_input_event(Event::Key(Key::Ctrl('w')));
+    tui.handle_input_event(Event::Key(Key::Ctrl('w')), &mut None);
     tui.draw();
 
     #[rustfmt::skip]
@@ -252,7 +252,7 @@ fn ctrl_w() {
     expect_screen(screen, &tui, 30, 3, Location::caller());
 
     println!("~~~~~~~~~~~~~~~~~~~~~~");
-    tui.handle_input_event(Event::Key(Key::Ctrl('w')));
+    tui.handle_input_event(Event::Key(Key::Ctrl('w')), &mut None);
     println!("~~~~~~~~~~~~~~~~~~~~~~");
     tui.draw();
 
@@ -273,7 +273,7 @@ fn ctrl_w() {
 
     expect_screen(screen, &tui, 30, 3, Location::caller());
 
-    tui.handle_input_event(Event::Key(Key::Ctrl('w')));
+    tui.handle_input_event(Event::Key(Key::Ctrl('w')), &mut None);
     tui.draw();
 
     #[rustfmt::skip]
@@ -305,11 +305,11 @@ fn test_text_field_wrap() {
 
     for _ in 0..37 {
         let event = term_input::Event::Key(Key::Char('a'));
-        tui.handle_input_event(event);
+        tui.handle_input_event(event, &mut None);
     }
     for _ in 0..5 {
         let event = term_input::Event::Key(Key::Char('b'));
-        tui.handle_input_event(event);
+        tui.handle_input_event(event, &mut None);
     }
 
     tui.draw();
@@ -351,7 +351,7 @@ fn test_text_field_wrap() {
     // the text field
     for _ in 0..6 {
         let event = term_input::Event::Key(Key::Backspace);
-        tui.handle_input_event(event);
+        tui.handle_input_event(event, &mut None);
     }
 
     tui.draw();
@@ -373,7 +373,7 @@ fn test_text_field_wrap() {
     tui.set_size(30, 8);
     for _ in 0..5 {
         let event = term_input::Event::Key(Key::Char('b'));
-        tui.handle_input_event(event);
+        tui.handle_input_event(event, &mut None);
     }
     tui.draw();
 
@@ -409,16 +409,16 @@ fn test_text_field_wrap() {
     // Wrapping on words - splits lines on whitespace
     for _ in 0..6 {
         let event = term_input::Event::Key(Key::Backspace);
-        tui.handle_input_event(event);
+        tui.handle_input_event(event, &mut None);
     }
     // InputLine cache gets invalidated after backspace, need to redraw to calculate.
     tui.draw();
     let event = term_input::Event::Key(Key::Char(' '));
-    tui.handle_input_event(event);
+    tui.handle_input_event(event, &mut None);
 
     for _ in 0..5 {
         let event = term_input::Event::Key(Key::Char('b'));
-        tui.handle_input_event(event);
+        tui.handle_input_event(event, &mut None);
     }
 
     tui.draw();