From 632e15015241855260e5d496a5c65955ef9be5fa Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 15 May 2021 12:50:25 -0700 Subject: [PATCH] Introduce notion of "wait handles" This is preparing to address the problem where fish cannot wait on a reaped job, because it only looks at the active job list. Introduce the idea of a "wait handle," which is a thing that `wait` can use to check if a job is finished. A job may produce its wait handle on demand, and parser_t will save the wait handle from wait-able jobs at the point they are reaped. This change merely introduces the idea; the next change makes builtin_wait start using it. --- src/builtin_bg.cpp | 2 +- src/parser.cpp | 26 ++++++++++++++++++++++++++ src/parser.h | 18 ++++++++++++++++++ src/proc.cpp | 29 +++++++++++++++++++++++++---- src/proc.h | 26 ++++++++++++++++++++++++-- 5 files changed, 94 insertions(+), 7 deletions(-) diff --git a/src/builtin_bg.cpp b/src/builtin_bg.cpp index d3ea75d0c..591456844 100644 --- a/src/builtin_bg.cpp +++ b/src/builtin_bg.cpp @@ -72,7 +72,7 @@ maybe_t builtin_bg(parser_t &parser, io_streams_t &streams, const wchar_t * } // The user specified at least one job to be backgrounded. - std::vector pids; + std::vector pids; // If one argument is not a valid pid (i.e. integer >= 0), fail without backgrounding anything, // but still print errors for all of them. diff --git a/src/parser.cpp b/src/parser.cpp index a4ee68509..4aaf4ecbc 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -553,6 +553,32 @@ void parser_t::job_add(shared_ptr job) { job_list.push_front(std::move(job)); } +void parser_t::save_wait_handle_for_completed_job(job_t *job) { + assert(job && job->is_completed() && "Job null or not completed"); + // Are we a background job with an external process? + if (!job->is_foreground() && job->has_external_proc()) { + rec_wait_handles.push_front(job->get_wait_handle(true /* create */)); + + // Limit how many background jobs we will remember. + // This is CHILD_MAX (controlled by _SC_CHILD_MAX) but we just hard code it. + // 1024 is zsh's fallback. + while (rec_wait_handles.size() > 1024) rec_wait_handles.pop_back(); + } + + // Mark the job as complete in its wait handle (but don't create it just for this). + if (auto wh = job->get_wait_handle(false /* create */)) { + wh->completed = true; + } +} + +void parser_t::wait_handle_remove(const wait_handle_ref_t &handle) { + // Note the handle may not be found, if we exceeded our wait handle limit. + auto iter = std::find(rec_wait_handles.begin(), rec_wait_handles.end(), handle); + if (iter != rec_wait_handles.end()) { + rec_wait_handles.erase(iter); + } +} + void parser_t::job_promote(job_t *job) { job_list_t::iterator loc; for (loc = job_list.begin(); loc != job_list.end(); ++loc) { diff --git a/src/parser.h b/src/parser.h index 9bc8748bf..6a910c210 100644 --- a/src/parser.h +++ b/src/parser.h @@ -248,8 +248,14 @@ class parser_t : public std::enable_shared_from_this { private: /// The current execution context. std::unique_ptr execution_context; + /// The jobs associated with this parser. job_list_t job_list; + + /// The list of recorded wait-handles. These are jobs that finished in the background, and have + /// been reaped, but may still be wait'ed on. + std::deque rec_wait_handles; + /// The list of blocks. This is a deque because we give out raw pointers to callers, who hold /// them across manipulating this stack. /// This is in "reverse" order: the topmost block is at the front. This enables iteration from @@ -361,6 +367,11 @@ class parser_t : public std::enable_shared_from_this { library_data_t &libdata() { return library_data; } const library_data_t &libdata() const { return library_data; } + /// Access the list of wait handles for jobs that have finished in the background. + const std::deque &get_recorded_wait_handles() const { + return rec_wait_handles; + } + /// Get and set the last proc statuses. int get_last_status() const { return vars().get_last_status(); } statuses_t get_last_statuses() const { return vars().get_last_statuses(); } @@ -397,6 +408,13 @@ class parser_t : public std::enable_shared_from_this { /// Returns the job with the given pid. job_t *job_get_from_pid(pid_t pid) const; + /// Given that a job has completed, check if it may be wait'ed on; if so add it to our list of + /// wait handles. + void save_wait_handle_for_completed_job(job_t *job); + + /// Remove a wait handle, if present in the list. + void wait_handle_remove(const wait_handle_ref_t &handle); + /// Returns a new profile item if profiling is active. The caller should fill it in. /// The parser_t will deallocate it. /// If profiling is not active, this returns nullptr. diff --git a/src/proc.cpp b/src/proc.cpp index a8b0b82fd..bace61f47 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -189,6 +189,21 @@ maybe_t job_t::get_statuses() const { return st; } +wait_handle_ref_t job_t::get_wait_handle(bool create) { + if (!wait_handle && create) { + wait_handle = std::make_shared(); + for (const auto &proc : processes) { + // Only external processes may be wait'ed upon. + if (proc->type != process_type_t::external) continue; + if (proc->pid > 0) { + wait_handle->pids.push_back(proc->pid); + } + wait_handle->proc_base_names.push_back(wbasename(proc->actual_cmd)); + } + } + return wait_handle; +} + void internal_proc_t::mark_exited(proc_status_t status) { assert(!exited() && "Process is already exited"); status_.store(status, std::memory_order_relaxed); @@ -668,11 +683,17 @@ static bool process_clean_after_marking(parser_t &parser, bool allow_interactive // Remove completed jobs. // Do this before calling out to user code in the event handler below, to ensure an event // handler doesn't remove jobs on our behalf. - auto should_remove = [&](const shared_ptr &j) { - return should_process_job(j) && j->is_completed(); - }; auto &jobs = parser.jobs(); - jobs.erase(std::remove_if(jobs.begin(), jobs.end(), should_remove), jobs.end()); + for (auto iter = jobs.begin(); iter != jobs.end();) { + const shared_ptr &j = *iter; + if (should_process_job(j) && j->is_completed()) { + // If this job finished in the background, we have to remember to wait on it. + parser.save_wait_handle_for_completed_job(j.get()); + iter = jobs.erase(iter); + } else { + ++iter; + } + } // Post pending exit events. for (const auto &evt : exit_events) { diff --git a/src/proc.h b/src/proc.h index 449608bf7..67d46de2b 100644 --- a/src/proc.h +++ b/src/proc.h @@ -287,8 +287,8 @@ class process_t { redirection_spec_list_t proc_redirection_specs_; }; -typedef std::unique_ptr process_ptr_t; -typedef std::vector process_list_t; +using process_ptr_t = std::unique_ptr; +using process_list_t = std::vector; /// A user-visible job ID. using job_id_t = int; @@ -297,6 +297,21 @@ using job_id_t = int; /// Every job has a unique positive value for this. using internal_job_id_t = uint64_t; +/// The bits of a job necessary to support 'wait'. +/// This may outlive the job. +struct wait_handle_t { + /// The list of pids of the processes in this job. + std::vector pids{}; + + /// The list of "base names" of the processes from the job. + /// For example if the job is "/bin/sleep" then this will be 'sleep'. + wcstring_list_t proc_base_names{}; + + /// Set to true when the job is completed. + bool completed{false}; +}; +using wait_handle_ref_t = std::shared_ptr; + /// A struct representing a job. A job is a pipeline of one or more processes. class job_t { public: @@ -379,6 +394,10 @@ class job_t { // This is never null and not changed after construction. job_group_ref_t group{}; + // The wait handle. This is constructed lazily, and cached. + // Do not access this directly, use the get_wait_handle() function below. + wait_handle_ref_t wait_handle{}; + /// \return the pgid for the job, based on the job group. /// This may be none if the job consists of just internal fish functions or builtins. /// This may also be fish itself. @@ -472,6 +491,9 @@ class job_t { /// \returns the statuses for this job. maybe_t get_statuses() const; + + /// \return the wait handle for the job, creating it if \p create is set. + wait_handle_ref_t get_wait_handle(bool create); }; /// Whether this shell is attached to a tty.