From 33f3c03dae49698b5c781c8181cc90a45866b380 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Thu, 20 May 2021 11:25:32 -0700 Subject: [PATCH] Allow on-job-exit handlers to be added for any pid in the job Prior to this change, a function with an on-job-exit event handler must be added with the pgid of the job. But sometimes the pgid of the job is fish itself (if job control is disabled) and the previous commit made last_pid an actual pid from the job, instead of its pgroup. Switch on-job-exit to accept any pid from the job (except fish itself). This allows it to be used directly with $last_pid, except that it now works if job control is off. This is implemented by "resolving" the pid to the internal job id at the point the event handler is added. Also switch to passing the last pid of the job, rather than its pgroup. This aligns better with $last_pid. --- CHANGELOG.rst | 1 + doc_src/cmds/function.rst | 2 +- src/builtin_function.cpp | 34 +++++++++++++++++++++++++--------- src/common.h | 7 +++++++ src/event.cpp | 19 ++++++++++--------- src/event.h | 16 +++++++++++++--- src/function.cpp | 2 +- src/proc.cpp | 6 ++++-- src/proc.h | 7 ------- src/wait_handle.h | 4 ++++ tests/checks/jobs.fish | 5 +++++ 11 files changed, 71 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 86c4dae97..ac2d35e10 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -30,6 +30,7 @@ Scripting improvements - ``$last_pid`` now reports the pid of the last process in the pipeline, allowing it to be used in scripts (:issue:`5036`, :issue:`5832`, :issue:`7721`). - ``process-exit`` event handlers now receive the same value as ``$status`` in all cases, instead of receiving -1 when the exit was due to a signal. - ``process-exit`` event handlers for pid 0 also received ``JOB_EXIT`` events; this has been fixed. +- ``job-exit`` event handlers may now be created with any of the pids from the job. The handler is passed the last pid in the job as its second argument, instead of the process group. Interactive improvements ------------------------- diff --git a/doc_src/cmds/function.rst b/doc_src/cmds/function.rst index 41d0ecb16..b5904aad3 100644 --- a/doc_src/cmds/function.rst +++ b/doc_src/cmds/function.rst @@ -30,7 +30,7 @@ The following options are available: - ``-v`` or ``--on-variable VARIABLE_NAME`` tells fish to run this function when the variable VARIABLE_NAME changes value. Note that fish makes no guarantees on any particular timing or even that the function will be run for every single ``set``. Rather it will be run when the variable has been set at least once, possibly skipping some values or being run when the variable has been set to the same value (except for universal variables set in other shells - only changes in the value will be picked up for those). -- ``-j PGID`` or ``--on-job-exit PGID`` tells fish to run this function when the job with group ID PGID exits. Instead of PGID, the string 'caller' can be specified. This is only legal when in a command substitution, and will result in the handler being triggered by the exit of the job which created this command substitution. +- ``-j PID`` or ``--on-job-exit PID`` tells fish to run this function when the job containing a child process with the given PID exits. Instead of PID, the string 'caller' can be specified. This is only legal when in a command substitution, and will result in the handler being triggered by the exit of the job which created this command substitution. - ``-p PID`` or ``--on-process-exit PID`` tells fish to run this function when the fish child process with process ID PID exits. Instead of a PID, for backward compatibility, diff --git a/src/builtin_function.cpp b/src/builtin_function.cpp index d72d77818..241e08dec 100644 --- a/src/builtin_function.cpp +++ b/src/builtin_function.cpp @@ -53,6 +53,18 @@ static const struct woption long_options[] = { {L"inherit-variable", required_argument, nullptr, 'V'}, {nullptr, 0, nullptr, 0}}; +/// \return the internal_job_id for a pid, or 0 if none. +/// This looks through both active and finished jobs. +static internal_job_id_t job_id_for_pid(pid_t pid, parser_t &parser) { + if (const auto *job = parser.job_get_from_pid(pid)) { + return job->internal_job_id; + } + if (wait_handle_ref_t wh = parser.get_wait_handles().get_by_pid(pid)) { + return wh->internal_job_id; + } + return 0; +} + static int parse_cmd_opts(function_cmd_opts_t &opts, int *optind, //!OCLINT(high ncss method) int argc, const wchar_t **argv, parser_t &parser, io_streams_t &streams) { const wchar_t *cmd = L"function"; @@ -127,7 +139,7 @@ static int parse_cmd_opts(function_cmd_opts_t &opts, int *optind, //!OCLINT(hig e.param1.pid = pid; } else { e.type = event_type_t::job_exit; - e.param1.pgid = pid; + e.param1.jobspec = {pid, job_id_for_pid(pid, parser)}; } } opts.events.push_back(e); @@ -281,15 +293,19 @@ maybe_t builtin_function(parser_t &parser, io_streams_t &streams, // If there is an --on-process-exit or --on-job-exit event handler for some pid, and that // process has already exited, run it immediately (#7210). for (const event_description_t &ed : opts.events) { - if ((ed.type == event_type_t::process_exit || ed.type == event_type_t::job_exit) && - ed.param1.pid != EVENT_ANY_PID) { - wait_handle_ref_t wh = parser.get_wait_handles().get_by_pid(ed.param1.pid); + if (ed.type == event_type_t::process_exit) { + pid_t pid = ed.param1.pid; + if (pid == EVENT_ANY_PID) continue; + wait_handle_ref_t wh = parser.get_wait_handles().get_by_pid(pid); if (wh && wh->completed) { - if (ed.type == event_type_t::process_exit) { - event_fire(parser, event_t::process_exit(ed.param1.pid, wh->status)); - } else { - event_fire(parser, event_t::job_exit(ed.param1.pgid)); - } + event_fire(parser, event_t::process_exit(pid, wh->status)); + } + } else if (ed.type == event_type_t::job_exit) { + pid_t pid = ed.param1.jobspec.pid; + if (pid == EVENT_ANY_PID) continue; + wait_handle_ref_t wh = parser.get_wait_handles().get_by_pid(pid); + if (wh && wh->completed) { + event_fire(parser, event_t::job_exit(pid, wh->internal_job_id)); } } } diff --git a/src/common.h b/src/common.h index 7adb90164..685f3599d 100644 --- a/src/common.h +++ b/src/common.h @@ -156,6 +156,13 @@ enum { }; typedef unsigned int escape_flags_t; +/// A user-visible job ID. +using job_id_t = int; + +/// The non user-visible, never-recycled job ID. +/// Every job has a unique positive value for this. +using internal_job_id_t = uint64_t; + /// Issue a debug message with printf-style string formating and automatic line breaking. The string /// will begin with the string \c program_name, followed by a colon and a whitespace. /// diff --git a/src/event.cpp b/src/event.cpp index 8cda47aa1..0c58c3a1d 100644 --- a/src/event.cpp +++ b/src/event.cpp @@ -120,9 +120,10 @@ static bool handler_matches(const event_handler_t &classv, const event_t &instan return classv.desc.param1.pid == instance.desc.param1.pid; } case event_type_t::job_exit: { - if (classv.desc.param1.pgid == EVENT_ANY_PID) return true; + const auto &jobspec = classv.desc.param1.jobspec; + if (jobspec.pid == EVENT_ANY_PID) return true; only_once = true; - return classv.desc.param1.pgid == instance.desc.param1.pgid; + return jobspec.internal_job_id == instance.desc.param1.jobspec.internal_job_id; } case event_type_t::caller_exit: { only_once = true; @@ -167,12 +168,12 @@ wcstring event_get_desc(const parser_t &parser, const event_t &evt) { } case event_type_t::job_exit: { - if (job_t *j = parser.job_get_from_pid(ed.param1.pid)) { + const auto &jobspec = ed.param1.jobspec; + if (const job_t *j = parser.job_get_from_pid(jobspec.pid)) { return format_string(_(L"exit handler for job %d, '%ls'"), j->job_id(), j->command_wcstr()); } else { - return format_string(_(L"exit handler for job with process group %d"), - ed.param1.pgid); + return format_string(_(L"exit handler for job with pid %d"), jobspec.pid); } } @@ -431,7 +432,7 @@ void event_print(io_streams_t &streams, const wcstring &type_filter) { case event_type_t::process_exit: return d1.param1.pid < d2.param1.pid; case event_type_t::job_exit: - return d1.param1.pgid < d2.param1.pgid; + return d1.param1.jobspec.pid < d2.param1.jobspec.pid; case event_type_t::caller_exit: return d1.param1.caller_id < d2.param1.caller_id; case event_type_t::variable: @@ -526,12 +527,12 @@ event_t event_t::process_exit(pid_t pid, int status) { } // static -event_t event_t::job_exit(pid_t pgid) { +event_t event_t::job_exit(pid_t pid, internal_job_id_t jid) { event_t evt{event_type_t::job_exit}; - evt.desc.param1.pgid = pgid; + evt.desc.param1.jobspec = {pid, jid}; evt.arguments.reserve(3); evt.arguments.push_back(L"JOB_EXIT"); - evt.arguments.push_back(to_string(pgid)); + evt.arguments.push_back(to_string(pid)); evt.arguments.push_back(L"0"); // historical return evt; } diff --git a/src/event.h b/src/event.h index 2e2319fd4..52049059e 100644 --- a/src/event.h +++ b/src/event.h @@ -43,6 +43,16 @@ extern const wchar_t *const event_filter_names[]; /// Properties of an event. struct event_description_t { + /// Helper type for on-job-exit events. + struct job_spec_t { + // pid requested by the event, or ANY_PID for all. + pid_t pid; + + // internal_job_id of the job to match. + // If this is 0, we match either all jobs (pid == ANY_PID) or no jobs (otherwise). + uint64_t internal_job_id; + }; + /// The event type. event_type_t type; @@ -50,12 +60,12 @@ struct event_description_t { /// /// signal: Signal number for signal-type events.Use EVENT_ANY_SIGNAL to match any signal /// pid: Process id for process-type events. Use EVENT_ANY_PID to match any pid. - /// pgid: Process id for job-type events. This value is positive (or EVENT_ANY_PID). + /// jobspec: Info for on-job-exit events. /// caller_id: Internal job id for caller_exit type events union { int signal; pid_t pid; - pid_t pgid; + job_spec_t jobspec; uint64_t caller_id; } param1{}; @@ -103,7 +113,7 @@ struct event_t { /// Create a JOB_EXIT event. The pgid should be positive. /// The reported status is always 0 for historical reasons. - static event_t job_exit(pid_t pgid); + static event_t job_exit(pid_t pgid, internal_job_id_t jid); /// Create a caller_exit event. static event_t caller_exit(uint64_t internal_job_id, int job_id); diff --git a/src/function.cpp b/src/function.cpp index 10db56869..f65d3414d 100644 --- a/src/function.cpp +++ b/src/function.cpp @@ -388,7 +388,7 @@ wcstring functions_def(const wcstring &name) { break; } case event_type_t::job_exit: { - append_format(out, L" --on-job-exit %d", d.param1.pgid); + append_format(out, L" --on-job-exit %d", d.param1.jobspec.pid); break; } case event_type_t::caller_exit: { diff --git a/src/proc.cpp b/src/proc.cpp index 6823e251a..22bb68d55 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -610,6 +610,7 @@ static void save_wait_handle_for_completed_job(const shared_ptr &job, for (auto &proc : job->processes) { if (wait_handle_ref_t wh = proc->get_wait_handle(false /* create */)) { wh->status = proc->status.status_value(); + wh->internal_job_id = job->internal_job_id; wh->completed = true; } } @@ -673,8 +674,9 @@ static bool process_clean_after_marking(parser_t &parser, bool allow_interactive // If this job already came from an event handler, // don't create an event or it's easy to get an infinite loop. if (!j->from_event_handler() && j->should_report_process_exits()) { - pid_t pgid = *j->get_pgid(); - exit_events.push_back(event_t::job_exit(pgid)); + if (auto last_pid = j->get_last_pid()) { + exit_events.push_back(event_t::job_exit(*last_pid, j->internal_job_id)); + } } // Caller exit events we still create, which anecdotally fixes `source (thing | psub)` // inside event handlers. This seems benign since this event is barely used (basically diff --git a/src/proc.h b/src/proc.h index 289cab57d..44367f777 100644 --- a/src/proc.h +++ b/src/proc.h @@ -298,13 +298,6 @@ class process_t { using process_ptr_t = std::unique_ptr; using process_list_t = std::vector; -/// A user-visible job ID. -using job_id_t = int; - -/// The non user-visible, never-recycled job ID. -/// Every job has a unique positive value for this. -using internal_job_id_t = uint64_t; - /// A struct representing a job. A job is a pipeline of one or more processes. class job_t { public: diff --git a/src/wait_handle.h b/src/wait_handle.h index d0fd79075..f36b890f2 100644 --- a/src/wait_handle.h +++ b/src/wait_handle.h @@ -23,6 +23,10 @@ struct wait_handle_t { /// The pid of this process. pid_t pid{}; + /// The internal job id of the job which contained this process. + /// This is initially 0; it is set when the job is completed. + internal_job_id_t internal_job_id{}; + /// The "base name" of this process. /// For example if the process is "/bin/sleep" then this will be 'sleep'. wcstring base_name{}; diff --git a/tests/checks/jobs.fish b/tests/checks/jobs.fish index 1df6e7afd..7c97f3b94 100644 --- a/tests/checks/jobs.fish +++ b/tests/checks/jobs.fish @@ -25,6 +25,11 @@ sleep 0.1 # (there should be none). ps -o stat | string match 'Z*' +# Verify disown can be used with last_pid, even if it is separate from the pgroup. +# This should silently succeed. +command true | sleep 0.5 & +disown $last_pid + jobs -q echo $status #CHECK: 1