From e95bcfb074e01698b04d1860c44352163c68bed8 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 8 Feb 2020 14:34:10 -0800 Subject: [PATCH] Teach a job to decide its job tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Job trees come in two flavors: “placeholders” for jobs which are only fish functions, and non-placeholders which need to track a pgid. This adds logic to allow a job to decide if its parent's job tree is appropriate, and allocating a new tree if not. --- src/builtin_eval.cpp | 2 +- src/exec.cpp | 38 ++++++++++++++++++++++---------------- src/exec.h | 2 +- src/expand.cpp | 2 +- src/io.h | 8 +++++--- src/operation_context.h | 7 ++++--- src/parse_execution.cpp | 3 ++- src/parser.cpp | 12 ++++++------ src/parser.h | 7 ++++--- src/proc.cpp | 36 +++++++++++++++++++++++++++++++++++- src/proc.h | 19 ++++++++++++++++--- 11 files changed, 97 insertions(+), 39 deletions(-) diff --git a/src/builtin_eval.cpp b/src/builtin_eval.cpp index 50d0849cd..b80fc3357 100644 --- a/src/builtin_eval.cpp +++ b/src/builtin_eval.cpp @@ -59,7 +59,7 @@ int builtin_eval(parser_t &parser, io_streams_t &streams, wchar_t **argv) { } int status = STATUS_CMD_OK; - auto res = parser.eval(new_cmd, ios, streams.parent_pgid); + auto res = parser.eval(new_cmd, ios, streams.job_tree); if (res.was_empty) { // Issue #5692, in particular, to catch `eval ""`, `eval "begin; end;"`, etc. // where we have an argument but nothing is executed. diff --git a/src/exec.cpp b/src/exec.cpp index 83adbb60c..1d1c00db0 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -59,7 +59,7 @@ pgroup_provenance_t get_pgroup_provenance(const shared_ptr &j, bool has_external = j->has_external_proc(); assert(first_proc_is_internal ? has_internal : has_external); - if (lineage.parent_pgid.has_value() && j->is_foreground()) { + if (lineage.job_tree && lineage.job_tree->get_pgid().has_value() && j->is_foreground()) { // 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, unless it's // backgrounded, in which case it should not inherit a pgroup. @@ -209,6 +209,7 @@ static void maybe_assign_pgid_from_child(const std::shared_ptr &j, pid_t // If our assignment mode is the first process, then assign it. if (j->pgid == INVALID_PID && j->pgroup_provenance == pgroup_provenance_t::first_external_proc) { + j->job_tree->set_pgid(child_pid); j->pgid = child_pid; } } @@ -659,7 +660,7 @@ static proc_performer_t get_performer_for_process(process_t *p, job_t *job, "Unexpected process type"); // Make a lineage for our children. job_lineage_t lineage; - lineage.parent_pgid = (job->pgid == INVALID_PID ? none() : maybe_t(job->pgid)); + lineage.job_tree = job->job_tree; lineage.block_io = io_chain; lineage.root_constructed = job->root_constructed; lineage.root_has_job_control = job->wants_job_control(); @@ -862,7 +863,7 @@ static bool exec_process_in_job(parser_t &parser, process_t *p, const std::share case process_type_t::builtin: { io_streams_t builtin_io_streams{stdout_read_limit}; - if (j->pgid != INVALID_PID) builtin_io_streams.parent_pgid = j->pgid; + builtin_io_streams.job_tree = j->job_tree; if (!exec_internal_builtin_proc(parser, p, pipe_read.get(), process_net_io_chain, builtin_io_streams)) { return false; @@ -956,13 +957,19 @@ bool exec_job(parser_t &parser, const shared_ptr &j, const job_lineage_t // Perhaps we know our pgroup already. assert(j->pgid == INVALID_PID && "Should not yet have a pid."); switch (j->pgroup_provenance) { - case pgroup_provenance_t::lineage: - assert(*lineage.parent_pgid != INVALID_PID && "pgid should be none, not INVALID_PID"); - j->pgid = *lineage.parent_pgid; + case pgroup_provenance_t::lineage: { + auto pgid = lineage.job_tree->get_pgid(); + assert(pgid && *pgid != INVALID_PID && "Should have valid pgid"); + j->pgid = *pgid; break; + } case pgroup_provenance_t::fish_itself: j->pgid = pgrp; + // TODO: should not need this 'if' here. Rationalize this. + if (! j->job_tree->is_placeholder()) { + j->job_tree->set_pgid(pgrp); + } break; // The remaining cases are all deferred until later. @@ -1114,15 +1121,15 @@ static void populate_subshell_output(wcstring_list_t *lst, const io_buffer_t &bu /// Execute \p cmd in a subshell in \p parser. If \p lst is not null, populate it with the output. /// Return $status in \p out_status. -/// If \p parent_pgid is set, any spawned commands should join that pgroup. +/// If \p job_tree is set, any spawned commands should join that job tree. /// If \p apply_exit_status is false, then reset $status back to its original value. /// \p is_subcmd controls whether we apply a read limit. /// \p break_expand is used to propagate whether the result should be "expansion breaking" in the /// sense that subshells used during string expansion should halt that expansion. \return the value /// of $status. -static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, maybe_t parent_pgid, - wcstring_list_t *lst, bool *break_expand, bool apply_exit_status, - bool is_subcmd) { +static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, + const job_tree_ref_t &job_tree, wcstring_list_t *lst, + bool *break_expand, bool apply_exit_status, bool is_subcmd) { ASSERT_IS_MAIN_THREAD(); auto &ld = parser.libdata(); @@ -1145,8 +1152,7 @@ static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, maybe_t *break_expand = true; return STATUS_CMD_ERROR; } - eval_res_t eval_res = - parser.eval(cmd, io_chain_t{bufferfill}, parent_pgid, block_type_t::subst); + eval_res_t eval_res = parser.eval(cmd, io_chain_t{bufferfill}, job_tree, block_type_t::subst); std::shared_ptr buffer = io_bufferfill_t::finish(std::move(bufferfill)); if (buffer->buffer().discarded()) { *break_expand = true; @@ -1165,24 +1171,24 @@ static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, maybe_t return eval_res.status.status_value(); } -int exec_subshell_for_expand(const wcstring &cmd, parser_t &parser, maybe_t parent_pgid, +int exec_subshell_for_expand(const wcstring &cmd, parser_t &parser, const job_tree_ref_t &job_tree, wcstring_list_t &outputs) { ASSERT_IS_MAIN_THREAD(); bool break_expand = false; - int ret = exec_subshell_internal(cmd, parser, parent_pgid, &outputs, &break_expand, true, true); + int ret = exec_subshell_internal(cmd, parser, job_tree, &outputs, &break_expand, true, true); // Only return an error code if we should break expansion. return break_expand ? ret : STATUS_CMD_OK; } int exec_subshell(const wcstring &cmd, parser_t &parser, bool apply_exit_status) { bool break_expand = false; - return exec_subshell_internal(cmd, parser, none(), nullptr, &break_expand, apply_exit_status, + return exec_subshell_internal(cmd, parser, nullptr, nullptr, &break_expand, apply_exit_status, false); } int exec_subshell(const wcstring &cmd, parser_t &parser, wcstring_list_t &outputs, bool apply_exit_status) { bool break_expand = false; - return exec_subshell_internal(cmd, parser, none(), &outputs, &break_expand, apply_exit_status, + return exec_subshell_internal(cmd, parser, nullptr, &outputs, &break_expand, apply_exit_status, false); } diff --git a/src/exec.h b/src/exec.h index 7eb657209..1ac767b63 100644 --- a/src/exec.h +++ b/src/exec.h @@ -31,7 +31,7 @@ int exec_subshell(const wcstring &cmd, parser_t &parser, wcstring_list_t &output /// "success" (even though the command may have failed), a non-zero return means that we should /// halt expansion. If the \p pgid is supplied, then any spawned external commands should join that /// pgroup. -int exec_subshell_for_expand(const wcstring &cmd, parser_t &parser, maybe_t parent_pgid, +int exec_subshell_for_expand(const wcstring &cmd, parser_t &parser, const job_tree_ref_t &job_tree, wcstring_list_t &outputs); /// Loops over close until the syscall was run without being interrupted. diff --git a/src/expand.cpp b/src/expand.cpp index 8165645d1..2f5881487 100644 --- a/src/expand.cpp +++ b/src/expand.cpp @@ -622,7 +622,7 @@ static expand_result_t expand_cmdsubst(wcstring input, const operation_context_t wcstring_list_t sub_res; const wcstring subcmd(paren_begin + 1, paren_end - paren_begin - 1); - int subshell_status = exec_subshell_for_expand(subcmd, *ctx.parser, ctx.parent_pgid, sub_res); + int subshell_status = exec_subshell_for_expand(subcmd, *ctx.parser, ctx.job_tree, sub_res); if (subshell_status != 0) { // TODO: Ad-hoc switch, how can we enumerate the possible errors more safely? const wchar_t *err; diff --git a/src/io.h b/src/io.h index bb9f3e9e8..92cfa2dfd 100644 --- a/src/io.h +++ b/src/io.h @@ -21,6 +21,8 @@ using std::shared_ptr; +class job_tree_t; + /// A simple set of FDs. struct fd_set_t { std::vector fds; @@ -462,10 +464,10 @@ struct io_streams_t { // Actual IO redirections. This is only used by the source builtin. Unowned. const io_chain_t *io_chain{nullptr}; - // The pgid of the job, if any. This enables builtins which run more code like eval() to share - // pgid. + // The job tree of the job, if any. This enables builtins which run more code like eval() to + // share pgid. // TODO: this is awkwardly placed, consider just embedding a lineage here. - maybe_t parent_pgid{}; + std::shared_ptr job_tree{}; // io_streams_t cannot be copied. io_streams_t(const io_streams_t &) = delete; diff --git a/src/operation_context.h b/src/operation_context.h index 6eb79831e..f72f7e0aa 100644 --- a/src/operation_context.h +++ b/src/operation_context.h @@ -7,6 +7,7 @@ class environment_t; class parser_t; +class job_tree_t; /// A common helper which always returns false. bool no_cancel(); @@ -29,10 +30,10 @@ class operation_context_t { // context itself. const environment_t &vars; - /// The pgid of the parental job. + /// The job tree of the parental job. /// This is used only when expanding command substitutions. If this is set, any jobs created by - /// the command substitions should use this pgid. - maybe_t parent_pgid{}; + /// the command substitions should use this tree. + std::shared_ptr job_tree{}; // A function which may be used to poll for cancellation. cancel_checker_t cancel_checker; diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 2935fcdd1..459bf3eef 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -1283,8 +1283,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. + // Set the pgroup assignment mode and job tree, now that the job is populated. job->pgroup_provenance = get_pgroup_provenance(job, lineage); + job->job_tree = job_tree_t::decide_tree_for_job(job.get(), lineage.job_tree); // Success. Give the job to the parser - it will clean it up. parser->job_add(job); diff --git a/src/parser.cpp b/src/parser.cpp index 87a000d0e..a0e3658fe 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -640,12 +640,12 @@ profile_item_t *parser_t::create_profile_item() { return result; } -eval_res_t parser_t::eval(const wcstring &cmd, const io_chain_t &io, maybe_t parent_pgid, +eval_res_t parser_t::eval(const wcstring &cmd, const io_chain_t &io, const job_tree_ref_t &job_tree, enum block_type_t block_type) { // Parse the source into a tree, if we can. parse_error_list_t error_list; if (parsed_source_ref_t ps = parse_source(cmd, parse_flag_none, &error_list)) { - return this->eval(ps, io, parent_pgid, block_type); + return this->eval(ps, io, job_tree, block_type); } else { // Get a backtrace. This includes the message. wcstring backtrace_and_desc; @@ -662,12 +662,12 @@ eval_res_t parser_t::eval(const wcstring &cmd, const io_chain_t &io, maybe_t parent_pgid, enum block_type_t block_type) { + const job_tree_ref_t &job_tree, enum block_type_t block_type) { assert(block_type == block_type_t::top || block_type == block_type_t::subst); if (!ps->tree.empty()) { job_lineage_t lineage; lineage.block_io = io; - lineage.parent_pgid = parent_pgid; + lineage.job_tree = job_tree; // Execute the first node. tnode_t start{&ps->tree, &ps->tree.front()}; return this->eval_node(ps, start, std::move(lineage), block_type); @@ -705,8 +705,8 @@ eval_res_t parser_t::eval_node(const parsed_source_ref_t &ps, tnode_t node, operation_context_t op_ctx = this->context(); block_t *scope_block = this->push_block(block_t::scope_block(block_type)); - // Propogate any parent pgid. - op_ctx.parent_pgid = lineage.parent_pgid; + // Propogate our job tree. + op_ctx.job_tree = lineage.job_tree; // Create and set a new execution context. using exc_ctx_ref_t = std::unique_ptr; diff --git a/src/parser.h b/src/parser.h index 6839158b3..1cb378a23 100644 --- a/src/parser.h +++ b/src/parser.h @@ -282,20 +282,21 @@ class parser_t : public std::enable_shared_from_this { /// /// \param cmd the string to evaluate /// \param io io redirections to perform on all started jobs - /// \param parent_pgid if set, the pgid to give to spawned jobs + /// \param job_tree if set, the job tree to give to spawned jobs. /// \param block_type The type of block to push on the block stack, which must be either 'top' /// or 'subst'. /// \param break_expand If not null, return by reference whether the error ought to be an expand /// error. This includes nested expand errors, and command-not-found. /// /// \return the result of evaluation. - eval_res_t eval(const wcstring &cmd, const io_chain_t &io, maybe_t parent_pgid = {}, + eval_res_t eval(const wcstring &cmd, const io_chain_t &io, const job_tree_ref_t &job_tree = {}, block_type_t block_type = block_type_t::top); /// Evaluate the parsed source ps. /// Because the source has been parsed, a syntax error is impossible. eval_res_t eval(const parsed_source_ref_t &ps, const io_chain_t &io, - maybe_t parent_pgid = {}, block_type_t block_type = block_type_t::top); + const job_tree_ref_t &job_tree = {}, + block_type_t block_type = block_type_t::top); /// Evaluates a node. /// The node type must be grammar::statement or grammar::job_list. diff --git a/src/proc.cpp b/src/proc.cpp index f20f56dc1..86aafe08c 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -235,7 +235,8 @@ void print_exit_warning_for_jobs(const job_list_t &jobs) { fputws(_(L"Use 'disown PID' to remove jobs from the list without terminating them.\n"), stdout); } -job_tree_t::job_tree_t(bool placeholder) : is_placeholder_(placeholder) {} +job_tree_t::job_tree_t(bool job_control, bool placeholder) + : job_control_(job_control), is_placeholder_(placeholder) {} void job_tree_t::set_pgid(pid_t pgid) { // TODO: thread safety? @@ -245,6 +246,39 @@ void job_tree_t::set_pgid(pid_t pgid) { pgid_ = pgid; } +maybe_t job_tree_t::get_pgid() const { return pgid_; } + +job_tree_ref_t job_tree_t::decide_tree_for_job(const job_t *job, const job_tree_ref_t &proposed) { + // Note there's three cases to consider: + // nullptr -> this is a root job, there is no inherited job tree + // placeholder -> we are running as part of a simple function execution, create a new job + // tree for any spawned jobs + // non-placeholder -> we are running as part of a real pipeline + // Decide if this job can use the placeholder tree. + // This is true if it's a simple foreground execution of an internal proc. + + bool can_use_placeholder = + job->is_foreground() && job->processes.size() == 1 && job->processes.front()->is_internal(); + + bool needs_new_tree = false; + if (!proposed) { + // We don't have a tree yet. + needs_new_tree = true; + } else if (!job->is_foreground()) { + // Background jobs always get a new tree. + needs_new_tree = true; + } else if (proposed->is_placeholder() && !can_use_placeholder) { + // We cannot use the placeholder tree for this job. + needs_new_tree = true; + } + + if (!needs_new_tree) { + return proposed; + } else { + return job_tree_ref_t(new job_tree_t(job->wants_job_control(), can_use_placeholder)); + } +} + void job_mark_process_as_failed(const std::shared_ptr &job, const process_t *failed_proc) { // The given process failed to even lift off (e.g. posix_spawn failed) and so doesn't have a // valid pid. Mark it and everything after it as dead. diff --git a/src/proc.h b/src/proc.h index 1f7f372cc..8085e3e37 100644 --- a/src/proc.h +++ b/src/proc.h @@ -179,14 +179,23 @@ class job_tree_t { /// Get the pgid, or none() if it has not been set. maybe_t get_pgid() const; + /// \return whether we want job control + bool wants_job_control() const { return job_control_; } + /// \return whether this is a placeholder. bool is_placeholder() const { return is_placeholder_; } + /// Given a job and a proposed job tree (possibly null), return the job tree to actually use. + /// The proposed tree is the tree from the parent job, or null if this is a root. + static job_tree_ref_t decide_tree_for_job(const job_t *job, + const job_tree_ref_t &proposed_tree); + private: maybe_t pgid_{}; + const bool job_control_; const bool is_placeholder_; - explicit job_tree_t(bool placeholder); + explicit job_tree_t(bool job_control, bool placeholder); }; /// A structure representing a single fish process. Contains variables for tracking process state @@ -321,10 +330,10 @@ void release_job_id(job_id_t jid); /// job_t. It is also important that job_t not contain this: because it stores block IO, it will /// extend the life of the IO which may prevent pipes from closing in a timely manner. See #6397. struct job_lineage_t { - /// The pgid of the parental job. + /// The job tree. /// If our job is "nested" as part of a function or block execution, and that function or block /// is part of a pipeline, then this may be set. - maybe_t parent_pgid{}; + job_tree_ref_t job_tree{}; /// Whether job control is on for the root. /// This is set if our job is nested as part of a function or block execution. @@ -446,6 +455,10 @@ class job_t { /// All the processes in this job. process_list_t processes; + // The tree containing this job. + // This is never null and not changed after construction. + job_tree_ref_t job_tree{}; + /// Process group ID for the process group that this job is running in. /// Set to a nonexistent, non-return-value of getpgid() integer by the constructor pid_t pgid{INVALID_PID};