Separate on-job-exit and and on-process-exit events

It is possible to run a function when a process exits via `function
--on-process-exit`, or when a job exits via `function --on-job-exits`.
Internally these were distinguished by the pid in the event: if it was
positive, then it was a process exit. If negative, it represents a pgid
and is a job exit. If zero, it fires for both jobs and processes, which is
pretty weird.

Switch to tracking these explicitly. Separate out the --on-process-exit
and --on-job-exit event types into separate types. Stop negating pgids as
well.

CHANGELOG.rst

@@ -28,6 +28,7 @@ Scripting improvements
 - fish gained a ``--no-config`` option to disable reading the configuration. This applies both to the user's and the admin's config.fish (typically in /etc/fish/config.fish) and snippets and also sets $fish_function_path to just the functions shipped with fish and disables universal variables and history. (:issue:`7921`, :issue:`1256`).
 - When universal variables are unavailable for some reason, setting a universal variable now sets a global variable instead (:issue:`7921`).
 - ``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.
 
 Interactive improvements
 -------------------------

src/builtin_function.cpp

@@ -113,7 +113,7 @@ static int parse_cmd_opts(function_cmd_opts_t &opts, int *optind,  //!OCLINT(hig
                     e.type = event_type_t::caller_exit;
                     e.param1.caller_id = caller_id;
                 } else if ((opt == 'p') && (wcscasecmp(w.woptarg, L"%self") == 0)) {
-                    e.type = event_type_t::exit;
+                    e.type = event_type_t::process_exit;
                     e.param1.pid = getpid();
                 } else {
                     pid_t pid = fish_wcstoi(w.woptarg);
@@ -122,9 +122,13 @@ static int parse_cmd_opts(function_cmd_opts_t &opts, int *optind,  //!OCLINT(hig
                                                   w.woptarg);
                         return STATUS_INVALID_ARGS;
                     }
-
-                    e.type = event_type_t::exit;
-                    e.param1.pid = (opt == 'j' ? -1 : 1) * abs(pid);
+                    if (opt == 'p') {
+                        e.type = event_type_t::process_exit;
+                        e.param1.pid = pid;
+                    } else {
+                        e.type = event_type_t::job_exit;
+                        e.param1.pgid = pid;
+                    }
                 }
                 opts.events.push_back(e);
                 break;
@@ -277,13 +281,14 @@ maybe_t<int> 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::exit && ed.param1.pid != EVENT_ANY_PID) {
-            wait_handle_ref_t wh = parser.get_wait_handles().get_by_pid(abs(ed.param1.pid));
+        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 (wh && wh->completed) {
-                if (ed.param1.pid > 0) {
+                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.pid));
+                    event_fire(parser, event_t::job_exit(ed.param1.pgid));
                 }
             }
         }

src/builtin_functions.cpp

@@ -189,6 +189,15 @@ static int report_function_metadata(const wchar_t *funcname, bool verbose, io_st
     return STATUS_CMD_OK;
 }
 
+/// \return whether a type filter is valid.
+static bool type_filter_valid(const wcstring &filter) {
+    if (filter.empty()) return true;
+    for (size_t i = 0; event_filter_names[i]; i++) {
+        if (filter == event_filter_names[i]) return true;
+    }
+    return false;
+}
+
 /// The functions builtin, used for listing and erasing functions.
 maybe_t<int> builtin_functions(parser_t &parser, io_streams_t &streams, const wchar_t **argv) {
     const wchar_t *cmd = argv[0];
@@ -253,15 +262,12 @@ maybe_t<int> builtin_functions(parser_t &parser, io_streams_t &streams, const wc
     }
 
     if (opts.handlers) {
-        maybe_t<event_type_t> type_filter{};
-        if (opts.handlers_type) {
-            type_filter = event_type_for_name(opts.handlers_type);
-            if (!type_filter) {
-                streams.err.append_format(_(L"%ls: Expected generic | variable | signal | exit | "
-                                            L"job-id for --handlers-type\n"),
-                                          cmd);
-                return STATUS_INVALID_ARGS;
-            }
+        wcstring type_filter = opts.handlers_type ? opts.handlers_type : L"";
+        if (!type_filter_valid(type_filter)) {
+            streams.err.append_format(_(L"%ls: Expected generic | variable | signal | exit | "
+                                        L"job-id for --handlers-type\n"),
+                                      cmd);
+            return STATUS_INVALID_ARGS;
         }
         event_print(streams, type_filter);
         return STATUS_CMD_OK;

src/event.cpp

@@ -114,11 +114,16 @@ static bool handler_matches(const event_handler_t &classv, const event_t &instan
         case event_type_t::variable: {
             return instance.desc.str_param1 == classv.desc.str_param1;
         }
-        case event_type_t::exit: {
+        case event_type_t::process_exit: {
             if (classv.desc.param1.pid == EVENT_ANY_PID) return true;
             only_once = true;
             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;
+            only_once = true;
+            return classv.desc.param1.pgid == instance.desc.param1.pgid;
+        }
         case event_type_t::caller_exit: {
             only_once = true;
             return classv.desc.param1.caller_id == instance.desc.param1.caller_id;
@@ -157,21 +162,18 @@ wcstring event_get_desc(const parser_t &parser, const event_t &evt) {
             return format_string(_(L"handler for variable '%ls'"), ed.str_param1.c_str());
         }
 
-        case event_type_t::exit: {
-            if (ed.param1.pid > 0) {
-                return format_string(_(L"exit handler for process %d"), ed.param1.pid);
+        case event_type_t::process_exit: {
+            return format_string(_(L"exit handler for process %d"), ed.param1.pid);
+        }
+
+        case event_type_t::job_exit: {
+            if (job_t *j = parser.job_get_from_pid(-ed.param1.pid)) {
+                return format_string(_(L"exit handler for job %d, '%ls'"), j->job_id(),
+                                     j->command_wcstr());
             } else {
-                // In events, PGIDs are stored as negative PIDs
-                job_t *j = parser.job_get_from_pid(-ed.param1.pid);
-                if (j) {
-                    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.pid);
-                }
+                return format_string(_(L"exit handler for job with process group %d"),
+                                     -ed.param1.pgid);
             }
-            DIE("Unreachable");
         }
 
         case event_type_t::caller_exit: {
@@ -369,38 +371,52 @@ void event_fire(parser_t &parser, const event_t &event) {
     }
 }
 
-/// Mapping between event type to name.
-/// Note we don't bother to sort this.
-struct event_type_name_t {
-    event_type_t type;
-    const wchar_t *name;
-};
-
-static const event_type_name_t events_mapping[] = {{event_type_t::signal, L"signal"},
-                                                   {event_type_t::variable, L"variable"},
-                                                   {event_type_t::exit, L"exit"},
-                                                   {event_type_t::caller_exit, L"caller-exit"},
-                                                   {event_type_t::generic, L"generic"}};
-
-maybe_t<event_type_t> event_type_for_name(const wcstring &name) {
-    for (const auto &em : events_mapping) {
-        if (name == em.name) {
-            return em.type;
-        }
-    }
-    return none();
-}
-
 static const wchar_t *event_name_for_type(event_type_t type) {
-    for (const auto &em : events_mapping) {
-        if (type == em.type) {
-            return em.name;
-        }
+    switch (type) {
+        case event_type_t::any:
+            return L"any";
+        case event_type_t::signal:
+            return L"signal";
+        case event_type_t::variable:
+            return L"variable";
+        case event_type_t::process_exit:
+            return L"process-exit";
+        case event_type_t::job_exit:
+            return L"job-exit";
+        case event_type_t::caller_exit:
+            return L"caller-exit";
+        case event_type_t::generic:
+            return L"generic";
     }
     return L"";
 }
 
-void event_print(io_streams_t &streams, maybe_t<event_type_t> type_filter) {
+const wchar_t *const event_filter_names[] = {L"signal",       L"variable", L"exit",
+                                             L"process-exit", L"job-exit", L"caller-exit",
+                                             L"generic",      nullptr};
+
+static bool filter_matches_event(const wcstring &filter, event_type_t type) {
+    if (filter.empty()) return true;
+    switch (type) {
+        case event_type_t::any:
+            return false;
+        case event_type_t::signal:
+            return filter == L"signal";
+        case event_type_t::variable:
+            return filter == L"variable";
+        case event_type_t::process_exit:
+            return filter == L"process-exit" || filter == L"exit";
+        case event_type_t::job_exit:
+            return filter == L"job-exit" || filter == L"exit";
+        case event_type_t::caller_exit:
+            return filter == L"process-exit" || filter == L"exit";
+        case event_type_t::generic:
+            return filter == L"generic";
+    }
+    DIE("Unreachable");
+}
+
+void event_print(io_streams_t &streams, const wcstring &type_filter) {
     event_handler_list_t tmp = *s_event_handlers.acquire();
     std::sort(tmp.begin(), tmp.end(),
               [](const shared_ptr<event_handler_t> &e1, const shared_ptr<event_handler_t> &e2) {
@@ -412,8 +428,10 @@ void event_print(io_streams_t &streams, maybe_t<event_type_t> type_filter) {
                   switch (d1.type) {
                       case event_type_t::signal:
                           return d1.signal < d2.signal;
-                      case event_type_t::exit:
+                      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;
                       case event_type_t::caller_exit:
                           return d1.param1.caller_id < d2.param1.caller_id;
                       case event_type_t::variable:
@@ -427,7 +445,7 @@ void event_print(io_streams_t &streams, maybe_t<event_type_t> type_filter) {
     maybe_t<event_type_t> last_type{};
     for (const shared_ptr<event_handler_t> &evt : tmp) {
         // If we have a filter, skip events that don't match.
-        if (type_filter && *type_filter != evt->desc.type) {
+        if (!filter_matches_event(type_filter, evt->desc.type)) {
             continue;
         }
 
@@ -441,7 +459,8 @@ void event_print(io_streams_t &streams, maybe_t<event_type_t> type_filter) {
                 streams.out.append_format(L"%ls %ls\n", sig2wcs(evt->desc.param1.signal),
                                           evt->function_name.c_str());
                 break;
-            case event_type_t::exit:
+            case event_type_t::process_exit:
+            case event_type_t::job_exit:
                 break;
             case event_type_t::caller_exit:
                 streams.out.append_format(L"caller-exit %ls\n", evt->function_name.c_str());
@@ -497,7 +516,7 @@ event_t event_t::variable(wcstring name, wcstring_list_t args) {
 
 // static
 event_t event_t::process_exit(pid_t pid, int status) {
-    event_t evt{event_type_t::exit};
+    event_t evt{event_type_t::process_exit};
     evt.desc.param1.pid = pid;
     evt.arguments.reserve(3);
     evt.arguments.push_back(L"PROCESS_EXIT");
@@ -508,8 +527,8 @@ event_t event_t::process_exit(pid_t pid, int status) {
 
 // static
 event_t event_t::job_exit(pid_t pgid) {
-    event_t evt{event_type_t::exit};
-    evt.desc.param1.pid = pgid;
+    event_t evt{event_type_t::job_exit};
+    evt.desc.param1.pgid = pgid;
     evt.arguments.reserve(3);
     evt.arguments.push_back(L"JOB_EXIT");
     evt.arguments.push_back(to_string(pgid));

src/event.h

@@ -27,14 +27,20 @@ enum class event_type_t {
     signal,
     /// An event triggered by a variable update.
     variable,
-    /// An event triggered by a job or process exit.
-    exit,
+    /// An event triggered by a process exit.
+    process_exit,
+    /// An event triggered by a job exit.
+    job_exit,
     /// An event triggered by a job exit, triggering the 'caller'-style events only.
     caller_exit,
     /// A generic event.
     generic,
 };
 
+/// Null-terminated list of valid event filter names.
+/// These are what are valid to pass to 'functions --handlers-type'
+extern const wchar_t *const event_filter_names[];
+
 /// Properties of an event.
 struct event_description_t {
     /// The event type.
@@ -43,13 +49,14 @@ struct event_description_t {
     /// The type-specific parameter. The int types are one of the following:
     ///
     /// 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. (Negative
-    /// values are used for PGIDs).
+    /// 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).
     /// caller_id: Internal job id for caller_exit type events
     union {
         int signal;
-        uint64_t caller_id;
         pid_t pid;
+        pid_t pgid;
+        uint64_t caller_id;
     } param1{};
 
     /// The string types are one of the following:
@@ -94,7 +101,7 @@ struct event_t {
     /// Create a PROCESS_EXIT event.
     static event_t process_exit(pid_t pid, int status);
 
-    /// Create a JOB_EXIT event. The pgid should be negative.
+    /// 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);
 
@@ -126,8 +133,8 @@ void event_fire_delayed(parser_t &parser);
 /// Enqueue a signal event. Invoked from a signal handler.
 void event_enqueue_signal(int signal);
 
-/// Print all events. If type_filter is not none(), only output events with that type.
-void event_print(io_streams_t &streams, maybe_t<event_type_t> type_filter);
+/// Print all events. If type_filter is not empty, only output events with that type.
+void event_print(io_streams_t &streams, const wcstring &type_filter);
 
 /// Returns a string describing the specified event.
 wcstring event_get_desc(const parser_t &parser, const event_t &e);
@@ -136,7 +143,4 @@ wcstring event_get_desc(const parser_t &parser, const event_t &e);
 void event_fire_generic(parser_t &parser, const wchar_t *name,
                         const wcstring_list_t *args = nullptr);
 
-/// Return the event type for a given name, or none.
-maybe_t<event_type_t> event_type_for_name(const wcstring &name);
-
 #endif

src/function.cpp

@@ -383,11 +383,12 @@ wcstring functions_def(const wcstring &name) {
                 append_format(out, L" --on-variable %ls", d.str_param1.c_str());
                 break;
             }
-            case event_type_t::exit: {
-                if (d.param1.pid > 0)
-                    append_format(out, L" --on-process-exit %d", d.param1.pid);
-                else
-                    append_format(out, L" --on-job-exit %d", -d.param1.pid);
+            case event_type_t::process_exit: {
+                append_format(out, L" --on-process-exit %d", d.param1.pid);
+                break;
+            }
+            case event_type_t::job_exit: {
+                append_format(out, L" --on-job-exit %d", d.param1.pgid);
                 break;
             }
             case event_type_t::caller_exit: {

src/proc.cpp

@@ -674,7 +674,7 @@ static bool process_clean_after_marking(parser_t &parser, bool allow_interactive
             // 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));
+                exit_events.push_back(event_t::job_exit(pgid));
             }
             // 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

tests/checks/signal.fish

@@ -28,6 +28,11 @@ function anychild --on-process-exit 0
     echo $argv[1] $argv[3]
 end
 
+function anyjob --on-job-exit 0
+    # Type and exit status
+    echo $argv[1] $argv[3]
+end
+
 echo "command false:"
 command false
 # CHECK: command false: