correct handling of SIGHUP by interactive fish

This is a partial fix for issue #3737. It only addresses the SIGHUP
aspect of the problem. Fixing SIGTERM is TBD.

src/reader.cpp

@@ -310,7 +310,7 @@ static struct termios tty_modes_for_external_cmds;
 static void reader_super_highlight_me_plenty(int highlight_pos_adjust = 0, bool no_io = false);
 
 /// Variable to keep track of forced exits - see \c reader_exit_forced();
-static int exit_forced;
+static bool exit_forced;
 
 /// Give up control of terminal.
 static void term_donate() {
@@ -360,7 +360,7 @@ static void term_steal() {
     invalidate_termsize();
 }
 
-int reader_exit_forced() { return exit_forced; }
+bool reader_exit_forced() { return exit_forced; }
 
 /// Given a command line and an autosuggestion, return the string that gets shown to the user.
 wcstring combine_command_and_autosuggestion(const wcstring &cmdline,
@@ -807,7 +807,7 @@ void restore_term_mode() {
 void reader_exit(int do_exit, int forced) {
     if (data) data->end_loop = do_exit;
     end_loop = do_exit;
-    if (forced) exit_forced = 1;
+    if (forced) exit_forced = true;
 }
 
 void reader_repaint_needed() {
@@ -2204,6 +2204,8 @@ bool shell_is_exiting() {
 /// This function is called when the main loop notices that end_loop has been set while in
 /// interactive mode. It checks if it is ok to exit.
 static void handle_end_loop() {
+    job_iterator_t jobs;
+
     if (!reader_exit_forced()) {
         const parser_t &parser = parser_t::principal_parser();
         for (size_t i = 0; i < parser.block_count(); i++) {
@@ -2213,23 +2215,22 @@ static void handle_end_loop() {
                 return;
             }
         }
-    }
 
-    bool bg_jobs = false;
+        bool bg_jobs = false;
-    job_iterator_t jobs;
+        while (job_t *j = jobs.next()) {
-    while (job_t *j = jobs.next()) {
+            if (!job_is_completed(j)) {
-        if (!job_is_completed(j)) {
+                bg_jobs = true;
-            bg_jobs = true;
+                break;
-            break;
+            }
         }
-    }
 
-    if (!data->prev_end_loop && bg_jobs) {
+        if (!data->prev_end_loop && bg_jobs) {
-        fputws(_(L"There are still jobs active (use the jobs command to see them).\n"), stdout);
+            fputws(_(L"There are still jobs active (use the jobs command to see them).\n"), stdout);
-        fputws(_(L"A second attempt to exit will terminate them.\n"), stdout);
+            fputws(_(L"A second attempt to exit will terminate them.\n"), stdout);
-        reader_exit(0, 0);
+            reader_exit(0, 0);
-        data->prev_end_loop = 1;
+            data->prev_end_loop = 1;
-        return;
+            return;
+        }
     }
 
     // Kill remaining jobs before exiting.
@@ -2523,7 +2524,7 @@ const wchar_t *reader_readline(int nchars) {
                 break;
             }
             case R_EOF: {
-                exit_forced = 1;
+                exit_forced = true;
                 data->end_loop = 1;
                 break;
             }

src/reader.h

@@ -195,7 +195,7 @@ bool shell_is_exiting();
 void reader_handle_sigint();
 
 /// This function returns true if fish is exiting by force, i.e. because stdin died.
-int reader_exit_forced();
+bool reader_exit_forced();
 
 /// Test if the given shell command contains errors. Uses parser_test for testing. Suitable for
 /// reader_set_test_function().

tests/exit.expect

@@ -0,0 +1,37 @@
+# vim: set filetype=expect:
+#
+# Test handling of the `exit` command.
+set pid [spawn $fish]
+expect_prompt
+
+# Verify that if we attempt to exit with a job in the background we get warned
+# about that job and are told to type `exit` a second time.
+send "sleep 111 &\r"
+expect_prompt
+send "exit\r"
+expect "There are still jobs active"
+expect "A second attempt to exit will terminate them."
+expect_prompt
+
+# Running anything other than `exit` should result in the same warning with
+# the shell still running.
+send "sleep 113 &\r"
+expect_prompt
+send "exit\r"
+expect "There are still jobs active"
+expect "A second attempt to exit will terminate them."
+expect_prompt
+
+# Verify that asking to exit a second time does so.
+send "exit\r"
+catch {expect default exp_continue} output
+wait
+
+# Verify all child processes have been killed. We don't use `-p $pid` because
+# if the shell has a bug the child processes might have been reparented to pid
+# 1 rather than killed.
+set status [catch {exec pgrep -l -f "sleep 11"} output]
+if {$status == 0} {
+    puts stderr "Commands spawned by the shell still running after `exit`"
+    puts stderr $output
+}

tests/signals.expect

@@ -0,0 +1,28 @@
+# vim: set filetype=expect:
+#
+# Test signal handling for interactive shells.
+
+# Verify that sending SIGHUP to the shell, such as will happen when the tty is
+# closed by the terminal, terminates the shell and the foreground command and
+# any background commands run from that shell.
+set pid [spawn $fish]
+expect_prompt
+send "sleep 130 &\r"
+expect_prompt
+send "sleep 131 &\r"
+expect_prompt
+send "sleep 132\r"
+exec -- kill -HUP $pid
+
+# Verify the spawned fish shell has exited.
+catch {expect default exp_continue} output
+wait
+
+# Verify all child processes have been killed. We don't use `-p $pid` because
+# if the shell has a bug the child processes might have been reparented to pid
+# 1 rather than killed.
+set status [catch {exec pgrep -l -f "sleep 13"} output]
+if {$status == 0} {
+    puts stderr "Commands spawned by the shell still running after SIGHUP"
+    puts stderr $output
+}