From 10da6df506149cc1ae3ae03316d7c2b260c1c418 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Wed, 29 Jan 2020 16:39:44 -0800 Subject: [PATCH] Factor out logic about how pgroups are assigned Introduce pgroup_provenance_t, a type which captures "where the pgroup comes from." This centralizes some logic around how pgroups are assigned, and it anticipates concurrent execution. --- src/exec.cpp | 115 ++++++++++++++++++---------------------- src/exec.h | 8 +-- src/parse_execution.cpp | 3 ++ src/proc.cpp | 26 +++++++++ src/proc.h | 30 +++++++++++ 5 files changed, 117 insertions(+), 65 deletions(-) diff --git a/src/exec.cpp b/src/exec.cpp index cf322f0d3..748a36fbb 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -52,6 +52,37 @@ /// Number of calls to fork() or posix_spawn(). static relaxed_atomic_t s_fork_count{0}; +pgroup_provenance_t get_pgroup_provenance(const shared_ptr &j, + const job_lineage_t &lineage) { + bool first_proc_is_internal = j->processes.front()->is_internal(); + bool has_internal = j->has_internal_proc(); + bool has_external = j->has_external_proc(); + assert(first_proc_is_internal ? has_internal : has_external); + + if (lineage.parent_pgid.has_value()) { + // Our lineage indicates a pgid. This means the job is "nested" as a function or block + // inside another job, which has a real pgroup. We're going to use that. + return pgroup_provenance_t::lineage; + } else if (!j->wants_job_control()) { + // This job doesn't need job control, it can just live in the fish pgroup. + return pgroup_provenance_t::fish_itself; + } else if (has_external && !first_proc_is_internal) { + // The first process is external, it will own the pgroup. + return pgroup_provenance_t::first_external_proc; + } else if (has_external && first_proc_is_internal) { + // The terminal owner has to be the process which is permitted to read from stdin. + // This is the first process in the pipeline. When executing, a process in the job will + // claim the pgrp if it's not set; therefore set it according to the first process. + // Only do this if there's an external process - see #6011. + return pgroup_provenance_t::fish_itself; + } else { + assert(has_internal && !has_external && "Should be internal only"); + // This job consists of only internal functions or builtins; we do not need to assign a + // pgroup (yet). + return pgroup_provenance_t::unassigned; + } +} + /// This function is executed by the child process created by a call to fork(). It should be called /// after \c child_setup_process. It calls execve to replace the fish process image with the command /// specified in \c p. It never returns. Called in a forked child! Do not allocate memory, etc. @@ -172,16 +203,11 @@ static void internal_exec(env_stack_t &vars, job_t *j, const io_chain_t &block_i } } -static void on_process_created(const std::shared_ptr &j, pid_t child_pid) { - // We only need to do this the first time a child is forked/spawned - if (j->pgid != INVALID_PID) { - return; - } - - if (j->wants_job_control()) { +/// If our pgroup assignment mode wants us to use the first external proc, then apply it here. +static void maybe_assign_pgid_from_child(const std::shared_ptr &j, pid_t child_pid) { + // If our assignment mode is the first process, then assign it. + if (j->pgid == INVALID_PID && j->pgroup_mode == pgroup_provenance_t::first_external_proc) { j->pgid = child_pid; - } else { - j->pgid = getpgrp(); } } @@ -327,7 +353,7 @@ static bool fork_child_for_process(const std::shared_ptr &job, process_t p->argv0()); p->pid = pid; - on_process_created(job, p->pid); + maybe_assign_pgid_from_child(job, p->pid); set_child_group(job.get(), p->pid); terminal_maybe_give_to_job(job.get(), false); return true; @@ -550,7 +576,7 @@ static bool exec_external_command(parser_t &parser, const std::shared_ptr // these are all things do_fork() takes care of normally (for forked processes): p->pid = pid; - on_process_created(j, p->pid); + maybe_assign_pgid_from_child(j, p->pid); // We explicitly don't call set_child_group() for spawned processes because that // a) isn't necessary, and b) causes issues like fish-shell/fish-shell#4715 @@ -565,13 +591,6 @@ static bool exec_external_command(parser_t &parser, const std::shared_ptr if (j->wants_job_control() && getpgid(p->pid) != j->pgid) { set_child_group(j.get(), p->pid); } -#else - // In do_fork, the pid of the child process is used as the group leader if j->pgid - // invalid, posix_spawn assigned the new group a pgid equal to its own id if - // j->pgid was invalid, so this is what we do instead of calling set_child_group - if (j->pgid == INVALID_PID) { - j->pgid = pid; - } #endif terminal_maybe_give_to_job(j.get(), false); @@ -893,41 +912,6 @@ static process_t *get_deferred_process(const shared_ptr &j) { return nullptr; } -/// \return true if fish should claim the process group for this job. -/// This is true if there is at least one external process and if the first process is fish code. -static bool should_claim_process_group_for_job(const shared_ptr &j) { - // Check if there's an external process. - // See #6011. - bool has_external = false; - for (const auto &p : j->processes) { - if (p->type == process_type_t::external) { - has_external = true; - break; - } - } - if (!has_external) { - return false; - } - - // Check the first process. - // The terminal owner has to be the process which is permitted to read from stdin. - // This is the first process in the pipeline. When executing, a process in the job will - // claim the pgrp if it's not set; therefore set it according to the first process. - switch (j->processes.front()->type) { - case process_type_t::builtin: - case process_type_t::function: - case process_type_t::block_node: - // These are run internal to fish. - return true; - case process_type_t::external: - case process_type_t::exec: - // External will get its own pgroup after fork. - // exec will retain the pgroup. - return false; - } - DIE("unreachable"); -} - bool exec_job(parser_t &parser, const shared_ptr &j, const job_lineage_t &lineage) { assert(j && "null job_t passed to exec_job!"); @@ -940,13 +924,6 @@ bool exec_job(parser_t &parser, const shared_ptr &j, const job_lineage_t return true; } - // If our lineage indicates a pgid, share it. - if (lineage.parent_pgid.has_value()) { - assert(*lineage.parent_pgid != INVALID_PID && - "parent pgid should be none, not INVALID_PID"); - j->pgid = *lineage.parent_pgid; - } - pid_t pgrp = getpgrp(); // Check to see if we should reclaim the foreground pgrp after the job finishes or stops. const bool reclaim_foreground_pgrp = (tcgetpgrp(STDIN_FILENO) == pgrp); @@ -957,8 +934,22 @@ bool exec_job(parser_t &parser, const shared_ptr &j, const job_lineage_t j->mut_flags().job_control = true; } - if (j->pgid == INVALID_PID && should_claim_process_group_for_job(j)) { - j->pgid = pgrp; + // Perhaps we know our pgroup already. + assert(j->pgid == INVALID_PID && "Should not yet have a pid."); + switch (j->pgroup_mode) { + case pgroup_provenance_t::lineage: + assert(*lineage.parent_pgid != INVALID_PID && "pgid should be none, not INVALID_PID"); + j->pgid = *lineage.parent_pgid; + break; + + case pgroup_provenance_t::fish_itself: + j->pgid = pgrp; + break; + + // The remaining cases are all deferred until later. + case pgroup_provenance_t::unassigned: + case pgroup_provenance_t::first_external_proc: + break; } const size_t stdout_read_limit = parser.libdata().read_limit; diff --git a/src/exec.h b/src/exec.h index e2bd6f907..640b65bbf 100644 --- a/src/exec.h +++ b/src/exec.h @@ -7,14 +7,12 @@ #include #include "common.h" +#include "proc.h" /// Pipe redirection error message. #define PIPE_ERROR _(L"An error occurred while setting up pipe") /// Execute the processes specified by \p j in the parser \p. -class job_t; -struct job_lineage_t; -class parser_t; bool exec_job(parser_t &parser, const std::shared_ptr &j, const job_lineage_t &lineage); /// Evaluate a command. @@ -37,4 +35,8 @@ int exec_subshell_for_expand(const wcstring &cmd, parser_t &parser, wcstring_lis /// Loops over close until the syscall was run without being interrupted. void exec_close(int fd); +/// Compute the "pgroup provenance" for a job. This is a description of how the pgroup is +/// assigned. It's factored out because the logic has subtleties, and this centralizes it. +pgroup_provenance_t get_pgroup_provenance(const std::shared_ptr &j, + const job_lineage_t &lineage); #endif diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 465d6ab6c..2d3dc761b 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -1302,6 +1302,9 @@ end_execution_reason_t parse_execution_context_t::run_1_job(tnode_t job_ // Clean up the job on failure or cancellation. if (pop_result == end_execution_reason_t::ok) { + // Set the pgroup assignment mode, now that the job is populated. + job->pgroup_mode = get_pgroup_provenance(job, lineage); + // Success. Give the job to the parser - it will clean it up. parser->job_add(job); diff --git a/src/proc.cpp b/src/proc.cpp index 6073ddf3c..3e2911179 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -256,6 +256,18 @@ void process_t::check_generations_before_launch() { gens_ = topic_monitor_t::principal().current_generations(); } +bool process_t::is_internal() const { + switch (type) { + case process_type_t::builtin: + case process_type_t::function: + case process_type_t::block_node: + return true; + case process_type_t::external: + case process_type_t::exec: + return false; + } +} + job_t::job_t(job_id_t job_id, const properties_t &props, const job_lineage_t &lineage) : properties(props), job_id_(job_id), @@ -270,6 +282,20 @@ void job_t::mark_constructed() { *constructed = true; } +bool job_t::has_internal_proc() const { + for (const auto &p : processes) { + if (p->is_internal()) return true; + } + return false; +} + +bool job_t::has_external_proc() const { + for (const auto &p : processes) { + if (!p->is_internal()) return true; + } + return false; +} + /// A list of pids/pgids that have been disowned. They are kept around until either they exit or /// we exit. Poll these from time-to-time to prevent zombie processes from happening (#5342). static owning_lock> s_disowned_pids; diff --git a/src/proc.h b/src/proc.h index 718fb17bb..6d28861aa 100644 --- a/src/proc.h +++ b/src/proc.h @@ -237,6 +237,9 @@ class process_t { /// launch. This helps us avoid spurious waitpid calls. void check_generations_before_launch(); + /// \return whether this process type is internal (block, function, or builtin). + bool is_internal() const; + /// Actual command to pass to exec in case of process_type_t::external or process_type_t::exec. wcstring actual_cmd; @@ -293,6 +296,24 @@ struct job_lineage_t { std::shared_ptr root_constructed{}; }; +/// A job has a mode which describes how its pgroup is assigned (before the value is known). +/// This is a constant property of the job. +enum class pgroup_provenance_t { + /// The job has no pgroup assignment. This is used for e.g. a simple function invocation with no + /// pipeline. + unassigned, + + /// The job's pgroup is fish's pgroup. This is used when fish needs to read from the terminal, + /// or if job control is disabled. + fish_itself, + + /// The job's pgroup will come from its first external process. + first_external_proc, + + /// The job's pgroup will come from its lineage. This is used for jobs that are run nested. + lineage, +}; + /// A struct represeting a job. A job is basically a pipeline of one or more processes and a couple /// of flags. class job_t { @@ -384,6 +405,9 @@ class job_t { /// Set to a nonexistent, non-return-value of getpgid() integer by the constructor pid_t pgid{INVALID_PID}; + /// How the above pgroup is assigned. This should be set at construction and not modified after. + pgroup_provenance_t pgroup_mode{}; + /// The id of this job. job_id_t job_id() const { return job_id_; } @@ -445,6 +469,12 @@ class job_t { /// Mark this job as constructed. The job must not have previously been marked as constructed. void mark_constructed(); + /// \return whether we have internal or external procs, respectively. + /// Internal procs are builtins, blocks, and functions. + /// External procs include exec and external. + bool has_internal_proc() const; + bool has_external_proc() const; + // Helper functions to check presence of flags on instances of jobs /// The job has been fully constructed, i.e. all its member processes have been launched bool is_constructed() const { return *constructed; }