fix: hang when starting with nu shell (#272)

This patch fixes a bug where shpool would hang when trying to
start a nu shell by default. This has to do with our startup
sentinal injection. A workaround exists where you can just
blank out the prompt prefix, but we should try to automatically
do that much when working with nu.

I should add a note to the troubleshooting wiki explaining this
so that nu shell users don't get confused as to why shpool is
not injecting a prompt automatically for them.

Fixes #269

Author: ethanpailes

libshpool/src/consts.rs

@@ -33,8 +33,8 @@ pub const STARTUP_SENTINEL: &str = "SHPOOL_STARTUP_SENTINEL";
 pub const PROMPT_SENTINEL: &str = "SHPOOL_PROMPT_SETUP_SENTINEL";
 
 // A magic env var which indicates that a `shpool daemon` invocation should
-// actually just print the given sentinal then exit. We do this because
-// using `echo` will cause the sentinal value to appear multiple times
+// actually just print the given sentinel then exit. We do this because
+// using `echo` will cause the sentinel value to appear multiple times
 // in the output stream. For the same reason, we don't set the value
 // to an actual sentianl, but instead either "startup" or "prompt".
 pub const SENTINEL_FLAG_VAR: &str = "SHPOOL__INTERNAL__PRINT_SENTINEL";

libshpool/src/daemon/prompt.rs

@@ -43,10 +43,6 @@ pub fn maybe_inject_prefix(
     prompt_prefix: &str,
     session_name: &str,
 ) -> anyhow::Result<()> {
-    if prompt_prefix.is_empty() {
-        return Ok(());
-    }
-
     let shell_pid = pty_master.child_pid().ok_or(anyhow!("no child pid"))?;
     // scan for the startup sentinel so we know it is safe to sniff the shell
     let mut pty_master = pty_master.is_parent().context("expected parent")?;
@@ -180,9 +176,9 @@ pub struct SentinelScanner {
 
 impl SentinelScanner {
     /// Create a new sentinel scanner.
-    pub fn new(sentinal: &str) -> Self {
+    pub fn new(sentinel: &str) -> Self {
         let mut scanner = Trie::new();
-        scanner.insert(sentinal.bytes(), ());
+        scanner.insert(sentinel.bytes(), ());
 
         SentinelScanner { scanner, cursor: TrieCursor::Start, num_matches: 0 }
     }

libshpool/src/daemon/server.rs

@@ -852,10 +852,16 @@ impl Server {
             }
         });
 
+        let prompt_prefix_is_blank =
+            self.config.get().prompt_prefix.as_ref().map(|p| p.is_empty()).unwrap_or(false);
+        let supports_sentinels =
+            header.cmd.is_none() && !prompt_prefix_is_blank && !does_not_support_sentinels(&shell);
+        info!("supports_sentianls={}", supports_sentinels);
+
         // Inject the prompt prefix, if any. For custom commands, avoid doing this
         // since we have no idea what the command is so the shell code probably won't
         // work.
-        if header.cmd.is_none() {
+        if supports_sentinels {
             info!("injecting prompt prefix");
             let prompt_prefix = self
                 .config
@@ -884,6 +890,7 @@ impl Server {
             heartbeat: heartbeat_tx,
             heartbeat_ack: heartbeat_ack_rx,
         }));
+
         let mut session_inner = shell::SessionInner {
             name: header.name.clone(),
             shell_to_client_ctl: Arc::clone(&shell_to_client_ctl),
@@ -894,7 +901,7 @@ impl Server {
             term_db,
             daily_messenger: Arc::clone(&self.daily_messenger),
             needs_initial_motd_dump: dump_motd_on_new_session,
-            custom_cmd: header.cmd.is_some(),
+            supports_sentinels,
         };
         let child_pid = session_inner.pty_master.child_pid().ok_or(anyhow!("no child pid"))?;
         session_inner.shell_to_client_join_h =
@@ -1055,6 +1062,13 @@ impl Server {
     }
 }
 
+// HACK: this is not a good way to detect shells that don't support our
+// sentinel injection approach, but it is better than just hanging when a
+// user tries to start one.
+fn does_not_support_sentinels(shell: &str) -> bool {
+    shell.ends_with("nu")
+}
+
 #[instrument(skip_all)]
 fn parse_connect_header(stream: &mut UnixStream) -> anyhow::Result<ConnectHeader> {
     let header: ConnectHeader = protocol::decode_from(stream).context("parsing header")?;

libshpool/src/daemon/shell.rs

@@ -106,7 +106,7 @@ pub struct SessionInner {
     pub term_db: Arc<termini::TermInfo>,
     pub daily_messenger: Arc<show_motd::DailyMessenger>,
     pub needs_initial_motd_dump: bool,
-    pub custom_cmd: bool,
+    pub supports_sentinels: bool,
 
     /// The join handle for the always-on background shell->client thread.
     /// Only wrapped in an option so we can spawn the thread after
@@ -202,9 +202,7 @@ impl SessionInner {
 
         // We only scan for the prompt sentinel if the user has not set up a
         // custom command or blanked out the prompt_prefix config option.
-        let prompt_prefix_is_blank =
-            self.config.get().prompt_prefix.as_ref().map(|p| p.is_empty()).unwrap_or(false);
-        let mut has_seen_prompt_sentinel = self.custom_cmd || prompt_prefix_is_blank;
+        let mut has_seen_prompt_sentinel = !self.supports_sentinels;
 
         let daily_messenger = Arc::clone(&self.daily_messenger);
         let mut needs_initial_motd_dump = self.needs_initial_motd_dump;

shpool/tests/attach.rs

@@ -1122,6 +1122,7 @@ fn prompt_prefix_bash() -> anyhow::Result<()> {
         let mut stdout = child.stdout.take().context("missing stdout")?;
         let mut stdout_str = String::from("");
         stdout.read_to_string(&mut stdout_str).context("slurping stdout")?;
+        eprintln!("stdout_str: {}", stdout_str);
         let stdout_re = Regex::new(".*session_name=sh1 prompt>.*")?;
         assert!(stdout_re.is_match(&stdout_str));