From 123f3e6f93afca1960cca00d72a9f811b34e9ca6 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 8 Feb 2020 15:12:58 -0800 Subject: [PATCH] Put job_id into job_tree Job IDs are really a property of a job tree, not individual jobs. Reflect that fact by migrating job IDs into job_tree. --- src/exec.cpp | 9 ------ src/parse_execution.cpp | 5 ++-- src/proc.cpp | 46 +++++++++++++++++++----------- src/proc.h | 62 +++++++++++++++++++++-------------------- 4 files changed, 65 insertions(+), 57 deletions(-) diff --git a/src/exec.cpp b/src/exec.cpp index d85fe865a..d70c726cd 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -662,7 +662,6 @@ static proc_performer_t get_performer_for_process(process_t *p, job_t *job, job_lineage_t lineage; lineage.job_tree = job->job_tree; lineage.block_io = io_chain; - lineage.root_constructed = job->root_constructed; if (p->type == process_type_t::block_node) { const parsed_source_ref_t &source = p->block_node_source; @@ -713,14 +712,6 @@ static bool exec_block_or_func_process(parser_t &parser, const std::shared_ptris_first_in_job && p->is_last_in_job && j->flags().foreground) { - j->mark_internal(); - } - // Get the process performer, and just execute it directly. // Do it in this scoped way so that the performer function can be eagerly deallocating releasing // its captured io chain. diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index a43aa23fa..262c31d2a 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -1260,7 +1260,7 @@ end_execution_reason_t parse_execution_context_t::run_1_job(tnode_t job_ props.from_event_handler = ld.is_event; props.job_control = wants_job_control; - shared_ptr job = std::make_shared(acquire_job_id(), props, this->lineage); + shared_ptr job = std::make_shared(props); job->tmodes = tmodes; job->mut_flags().foreground = !job_node_is_background(job_node); @@ -1285,7 +1285,8 @@ end_execution_reason_t parse_execution_context_t::run_1_job(tnode_t job_ if (pop_result == end_execution_reason_t::ok) { // 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); + job_tree_t::populate_tree_for_job(job.get(), lineage.job_tree); + assert(job->job_tree && "Should have a job tree"); // 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 86aafe08c..9d912b433 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -149,6 +149,12 @@ bool job_t::should_report_process_exits() const { return false; } + // Only report root job exits. + // For example in `ls | begin ; cat ; end` we don't need to report the cat sub-job. + if (!flags().is_tree_root) { + return false; + } + // Return whether we have an external process. for (const auto &p : this->processes) { if (p->type == process_type_t::external) { @@ -158,7 +164,7 @@ bool job_t::should_report_process_exits() const { return false; } -bool job_t::job_chain_is_fully_constructed() const { return *root_constructed; } +bool job_t::job_chain_is_fully_constructed() const { return job_tree->is_root_constructed(); } bool job_t::signal(int signal) { // Presumably we are distinguishing between the two cases below because we do @@ -236,7 +242,15 @@ void print_exit_warning_for_jobs(const job_list_t &jobs) { } job_tree_t::job_tree_t(bool job_control, bool placeholder) - : job_control_(job_control), is_placeholder_(placeholder) {} + : job_control_(job_control), + is_placeholder_(placeholder), + job_id_(placeholder ? -1 : acquire_job_id()) {} + +job_tree_t::~job_tree_t() { + if (job_id_ > 0) { + release_job_id(job_id_); + } +} void job_tree_t::set_pgid(pid_t pgid) { // TODO: thread safety? @@ -248,7 +262,7 @@ void job_tree_t::set_pgid(pid_t 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) { +void job_tree_t::populate_tree_for_job(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 @@ -256,7 +270,6 @@ job_tree_ref_t job_tree_t::decide_tree_for_job(const job_t *job, const job_tree_ // 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(); @@ -272,10 +285,13 @@ job_tree_ref_t job_tree_t::decide_tree_for_job(const job_t *job, const job_tree_ needs_new_tree = true; } + job->mut_flags().is_tree_root = needs_new_tree; + if (!needs_new_tree) { - return proposed; + job->job_tree = proposed; } else { - return job_tree_ref_t(new job_tree_t(job->wants_job_control(), can_use_placeholder)); + job->job_tree = + job_tree_ref_t(new job_tree_t(job->wants_job_control(), can_use_placeholder)); } } @@ -354,19 +370,17 @@ static uint64_t next_internal_job_id() { return ++s_next; } -job_t::job_t(job_id_t job_id, const properties_t &props, const job_lineage_t &lineage) - : properties(props), - job_id_(job_id), - internal_job_id(next_internal_job_id()), - root_constructed(lineage.root_constructed ? lineage.root_constructed : this->constructed) {} +job_t::job_t(const properties_t &props) + : properties(props), internal_job_id(next_internal_job_id()) {} -job_t::~job_t() { - if (job_id_ != -1) release_job_id(job_id_); -} +job_t::~job_t() = default; void job_t::mark_constructed() { assert(!is_constructed() && "Job was already constructed"); - *constructed = true; + mut_flags().constructed = true; + if (flags().is_tree_root) { + job_tree->mark_root_constructed(); + } } bool job_t::has_internal_proc() const { @@ -944,7 +958,7 @@ void job_t::continue_job(parser_t &parser, bool reclaim_foreground_pgrp, bool se mut_flags().notified = false; FLOGF(proc_job_run, L"%ls job %d, gid %d (%ls), %ls, %ls", - send_sigcont ? L"Continue" : L"Start", job_id_, pgid, command_wcstr(), + send_sigcont ? L"Continue" : L"Start", job_id(), pgid, command_wcstr(), is_completed() ? L"COMPLETED" : L"UNCOMPLETED", parser.libdata().is_interactive ? L"INTERACTIVE" : L"NON-INTERACTIVE"); diff --git a/src/proc.h b/src/proc.h index 799ae80b4..2c40cac7d 100644 --- a/src/proc.h +++ b/src/proc.h @@ -154,6 +154,12 @@ class internal_proc_t { /// function enum { INVALID_PID = -2 }; +/// A job ID, corresponding to what is printed in 'jobs'. +/// 1 is the first valid job ID. +using job_id_t = int; +job_id_t acquire_job_id(void); +void release_job_id(job_id_t jid); + /// job_tree_t is conceptually similar to the idea of a process group. It represents data which /// is shared among all of the "subjobs" that may be spawned by a single job. /// For example, two fish functions in a pipeline may themselves spawn multiple jobs, but all will @@ -185,15 +191,27 @@ class job_tree_t { /// \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. + /// \return the job ID, or -1 if none. + job_id_t get_id() const { return job_id_; } + + /// Mark the root as constructed. + /// This is used to avoid reaping a process group leader while there are still procs that may + /// want to enter its group. + void mark_root_constructed() { root_constructed_ = true; }; + bool is_root_constructed() const { return root_constructed_; } + + /// Given a job and a proposed job tree (possibly null), populate the job's tree. /// 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); + static void populate_tree_for_job(job_t *job, const job_tree_ref_t &proposed_tree); + + ~job_tree_t(); private: maybe_t pgid_{}; const bool job_control_; const bool is_placeholder_; + const job_id_t job_id_; + relaxed_atomic_bool_t root_constructed_{}; explicit job_tree_t(bool job_control, bool placeholder); }; @@ -338,10 +356,6 @@ struct job_lineage_t { /// The IO chain associated with any block containing this job. /// For example, in `begin; foo ; end < file.txt` this would have the 'file.txt' IO. io_chain_t block_io{}; - - /// A shared pointer indicating that the entire tree of jobs is safe to disown. - /// This is set to true by the "root" job after it is constructed. - std::shared_ptr root_constructed{}; }; /// A job has a mode which describes how its pgroup is assigned (before the value is known). @@ -390,15 +404,12 @@ class job_t { /// messages about job status on the terminal. wcstring command_str; - /// The job_id for this job. - job_id_t job_id_; - // No copying. job_t(const job_t &rhs) = delete; void operator=(const job_t &) = delete; public: - job_t(job_id_t job_id, const properties_t &props, const job_lineage_t &lineage); + explicit job_t(const properties_t &props); ~job_t(); /// Returns the command as a wchar_t *. */ @@ -464,35 +475,22 @@ class job_t { /// The id of this job. /// This is user-visible, is recycled, and may be -1. - job_id_t job_id() const { return job_id_; } + job_id_t job_id() const { return job_tree->get_id(); } /// A non-user-visible, never-recycled job ID. const internal_job_id_t internal_job_id; - /// Mark this job as internal. Internal jobs' job_ids are removed from the - /// list of jobs so that, among other things, they don't take a job_id - /// entry. - void mark_internal() { - release_job_id(job_id_); - job_id_ = -1; - } - /// The saved terminal modes of this job. This needs to be saved so that we can restore the /// terminal to the same state after temporarily taking control over the terminal when a job /// stops. struct termios tmodes {}; - /// Whether the specified job is completely constructed, i.e. completely parsed, and every - /// process in the job has been forked, etc. - /// This is a shared_ptr because it may be passed to child jobs through the lineage. - const std::shared_ptr constructed = - std::make_shared(false); - - /// Whether the root job is constructed; this may share a reference with 'constructed'. - const std::shared_ptr root_constructed; - /// Flags associated with the job. struct flags_t { + /// Whether the specified job is completely constructed: every process in the job has been + /// forked, etc. + bool constructed{false}; + /// Whether the user has been told about stopped job. bool notified{false}; @@ -507,6 +505,10 @@ class job_t { /// Whether to print timing for this job. bool has_time_prefix{false}; + + // Indicates that we are the "tree root." Any other jobs using this tree are nested. + bool is_tree_root{false}; + } job_flags{}; /// Access the job flags. @@ -532,7 +534,7 @@ class job_t { // 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; } + bool is_constructed() const { return flags().constructed; } /// The job was launched in the foreground and has control of the terminal bool is_foreground() const { return flags().foreground; } /// The job is complete, i.e. all its member processes have been reaped