Refactor job control to make functions act like their names imply

The job control functions were a bit messy, in particular
`set_child_group`'s name would imply that all it does is set the child
group, but in reality it used to set the child group (via `setpgid`),
set the job's pgrp if it hasn't been set, and possibly assign control of
the terminal to the newly-created job.

These have been split into separate functions. Now `set_child_group`
does just (and only) that, `maybe_assign_terminal` might assign the
terminal to the new pgrp, and `on_process_created` is used to set the
job properties the first time an external process is created. This might
also speed things up (but probably not noticeably) as there are no more
repeated calls to `getpgrp()` if JOB_CONTROL is not set.

Additionally, this closes #4715 by no longer unconditionally calling
`setpgid` on all new processes, including those created by `posix_spawn`
which does not need this since the child's pgrep is set at in the
arguments to that API call.

src/exec.cpp

@@ -499,6 +499,19 @@ static bool exec_internal_builtin_proc(parser_t &parser, job_t *j, process_t *p,
     return true;  // "success"
 }
 
+void on_process_created(job_t *j, pid_t child_pid) {
+    // We only need to do this the first time a child is forked/spawned
+    if (j->pgid != -2) {
+        return;
+    }
+
+    if (j->get_flag(JOB_CONTROL)) {
+        j->pgid = child_pid;
+    } else {
+        j->pgid = getpgrp();
+    }
+}
+
 void exec_job(parser_t &parser, job_t *j) {
     pid_t pid = 0;
 
@@ -587,7 +600,9 @@ void exec_job(parser_t &parser, job_t *j) {
             // Parent
             debug(2, L"Fork #%d, pid %d: keepalive fork for '%ls'", g_fork_count, keepalive.pid,
                   j->command_wcstr());
+            on_process_created(j, keepalive.pid);
             set_child_group(j, keepalive.pid);
+            maybe_assign_terminal(j);
         }
     }
 
@@ -732,6 +747,8 @@ void exec_job(parser_t &parser, job_t *j) {
         // This is the io_streams we pass to internal builtins.
         std::unique_ptr<io_streams_t> builtin_io_streams(new io_streams_t(stdout_read_limit));
 
+        // We fork in several different places. Each time the same code must be executed, so unify
+        // it all here.
         auto do_fork = [&j, &p, &pid, &exec_error, &process_net_io_chain,
                         &child_forked](bool drain_threads, const char *fork_type,
                                        std::function<void()> child_action) -> bool {
@@ -744,22 +761,25 @@ void exec_job(parser_t &parser, job_t *j) {
                 setup_child_process(p, process_net_io_chain);
                 child_action();
                 DIE("Child process returned control to do_fork lambda!");
-            } else {
-                if (pid < 0) {
-                    debug(1, L"Failed to fork %s!\n", fork_type);
-                    job_mark_process_as_failed(j, p);
-                    exec_error = true;
-                    return false;
-                }
-                // This is the parent process. Store away information on the child, and
-                // possibly give it control over the terminal.
-                debug(2, L"Fork #%d, pid %d: %s for '%ls'", g_fork_count, pid, fork_type,
-                      p->argv0());
-                child_forked = true;
-                p->pid = pid;
-                set_child_group(j, p->pid);
             }
 
+            if (pid < 0) {
+                debug(1, L"Failed to fork %s!\n", fork_type);
+                job_mark_process_as_failed(j, p);
+                exec_error = true;
+                return false;
+            }
+
+            // This is the parent process. Store away information on the child, and
+            // possibly give it control over the terminal.
+            debug(2, L"Fork #%d, pid %d: %s for '%ls'", g_fork_count, pid, fork_type, p->argv0());
+            child_forked = true;
+
+            p->pid = pid;
+            on_process_created(j, p->pid);
+            set_child_group(j, p->pid);
+            maybe_assign_terminal(j);
+
             return true;
         };
 
@@ -1054,9 +1074,14 @@ void exec_job(parser_t &parser, job_t *j) {
                     if (pid == 0) {
                         job_mark_process_as_failed(j, p);
                         exec_error = true;
-                    } else {
-                        child_spawned = true;
+                        break;
                     }
+
+                    // these are all things do_fork() takes care of normally:
+                    p->pid = pid;
+                    child_spawned = true;
+                    on_process_created(j, p->pid);
+                    maybe_assign_terminal(j);
                 } else
 #endif
                 {
@@ -1066,10 +1091,6 @@ void exec_job(parser_t &parser, job_t *j) {
                     }
                 }
 
-                // This is the parent process. Store away information on the child, and possibly
-                // give it control over the terminal.
-                p->pid = pid;
-                set_child_group(j, p->pid);
                 break;
             }
 

src/postfork.cpp

@@ -105,24 +105,23 @@ bool child_set_group(job_t *j, process_t *p) {
 /// group in the case of JOB_CONTROL, and we can give the new process group control of the terminal
 /// if it's to run in the foreground.
 bool set_child_group(job_t *j, pid_t child_pid) {
-    bool retval = true;
-
     if (j->get_flag(JOB_CONTROL)) {
-        // New jobs have the pgid set to -2
-        if (j->pgid == -2) {
-            j->pgid = child_pid;
-        }
         // The parent sets the child's group. This incurs the well-known unavoidable race with the
         // child exiting, so ignore ESRCH and EPERM (in case the pid was recycled).
         if (setpgid(child_pid, j->pgid) < 0) {
             if (errno != ESRCH && errno != EPERM) {
                 safe_perror("setpgid");
+                return false;
             }
         }
     } else {
         j->pgid = getpgrp();
     }
 
+    return true;
+}
+
+bool maybe_assign_terminal(job_t *j) {
     if (j->get_flag(JOB_TERMINAL) && j->get_flag(JOB_FOREGROUND)) {  //!OCLINT(early exit)
         if (tcgetpgrp(STDIN_FILENO) == j->pgid) {
             // We've already assigned the process group control of the terminal when the first
@@ -133,11 +132,11 @@ bool set_child_group(job_t *j, pid_t child_pid) {
             debug(4, L"Process group %d already has control of terminal\n", j->pgid);
         } else {
             // No need to duplicate the code here, a function already exists that does just this.
-            retval = terminal_give_to_job(j, false /*new job, so not continuing*/);
+            return terminal_give_to_job(j, false /*new job, so not continuing*/);
         }
     }
 
-    return retval;
+    return true;
 }
 
 /// Set up a childs io redirections. Should only be called by setup_child_process(). Does the

src/postfork.h

@@ -20,6 +20,7 @@ class process_t;
 
 bool set_child_group(job_t *j, pid_t child_pid);  // called by parent
 bool child_set_group(job_t *j, process_t *p);     // called by child
+bool maybe_assign_terminal(job_t *j);
 
 /// Initialize a new child process. This should be called right away after forking in the child
 /// process. If job control is enabled for this job, the process is put in the process group of the