diff --git a/src/builtin_function.cpp b/src/builtin_function.cpp index cc4bf816c..a4502a1a7 100644 --- a/src/builtin_function.cpp +++ b/src/builtin_function.cpp @@ -104,19 +104,15 @@ static int parse_cmd_opts(function_cmd_opts_t &opts, int *optind, //!OCLINT(hig event_description_t e(event_type_t::any); if ((opt == 'j') && (wcscasecmp(w.woptarg, L"caller") == 0)) { - job_id_t job_id = -1; - - if (parser.libdata().is_subshell) { - job_id = parser.libdata().caller_job_id; - } - - if (job_id == -1) { + internal_job_id_t caller_id = + parser.libdata().is_subshell ? parser.libdata().caller_id : 0; + if (caller_id == 0) { streams.err.append_format( _(L"%ls: Cannot find calling job for event handler"), cmd); return STATUS_INVALID_ARGS; } - e.type = event_type_t::job_exit; - e.param1.job_id = job_id; + e.type = event_type_t::caller_exit; + e.param1.caller_id = caller_id; } else if ((opt == 'p') && (wcscasecmp(w.woptarg, L"%self") == 0)) { pid = getpid(); e.type = event_type_t::exit; diff --git a/src/builtin_functions.cpp b/src/builtin_functions.cpp index 951a1d5b0..a57683243 100644 --- a/src/builtin_functions.cpp +++ b/src/builtin_functions.cpp @@ -183,8 +183,8 @@ static wcstring functions_def(const wcstring &name) { append_format(out, L" --on-job-exit %d", -d.param1.pid); break; } - case event_type_t::job_exit: { - const job_t *j = job_t::from_job_id(d.param1.job_id); + case event_type_t::caller_exit: { + const job_t *j = job_t::from_job_id(d.param1.caller_id); if (j) append_format(out, L" --on-job-exit %d", j->pgid); break; } diff --git a/src/event.cpp b/src/event.cpp index f3bae6d13..fffc625bc 100644 --- a/src/event.cpp +++ b/src/event.cpp @@ -113,8 +113,8 @@ static bool handler_matches(const event_handler_t &classv, const event_t &instan if (classv.desc.param1.pid == EVENT_ANY_PID) return true; return classv.desc.param1.pid == instance.desc.param1.pid; } - case event_type_t::job_exit: { - return classv.desc.param1.job_id == instance.desc.param1.job_id; + case event_type_t::caller_exit: { + return classv.desc.param1.caller_id == instance.desc.param1.caller_id; } case event_type_t::generic: { return classv.desc.str_param1 == instance.desc.str_param1; @@ -167,13 +167,13 @@ wcstring event_get_desc(const event_t &evt) { DIE("Unreachable"); } - case event_type_t::job_exit: { - job_t *j = job_t::from_job_id(ed.param1.job_id); + case event_type_t::caller_exit: { + job_t *j = job_t::from_job_id(ed.param1.caller_id); 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 job id %d"), ed.param1.job_id); + return format_string(_(L"exit handler for job with job id %d"), ed.param1.caller_id); } break; } @@ -351,7 +351,7 @@ struct event_type_name_t { 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::job_exit, L"job-id"}, + {event_type_t::caller_exit, L"job-id"}, {event_type_t::generic, L"generic"}}; maybe_t event_type_for_name(const wcstring &name) { @@ -386,8 +386,8 @@ void event_print(io_streams_t &streams, maybe_t type_filter) { return d1.signal < d2.signal; case event_type_t::exit: return d1.param1.pid < d2.param1.pid; - case event_type_t::job_exit: - return d1.param1.job_id < d2.param1.job_id; + case event_type_t::caller_exit: + return d1.param1.caller_id < d2.param1.caller_id; case event_type_t::variable: case event_type_t::any: case event_type_t::generic: @@ -414,7 +414,7 @@ void event_print(io_streams_t &streams, maybe_t type_filter) { evt->function_name.c_str()); break; case event_type_t::exit: - case event_type_t::job_exit: + case event_type_t::caller_exit: streams.out.append_format(L"%d %ls\n", evt->desc.param1, evt->function_name.c_str()); break; diff --git a/src/event.h b/src/event.h index 13a6dd677..bff6fe0e2 100644 --- a/src/event.h +++ b/src/event.h @@ -29,8 +29,8 @@ enum class event_type_t { variable, /// An event triggered by a job or process exit. exit, - /// An event triggered by a job exit. - job_exit, + /// An event triggered by a caller exit. + caller_exit, /// A generic event. generic, }; @@ -44,10 +44,11 @@ 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. (Negative - /// values are used for PGIDs). job_id: Job id for EVENT_JOB_ID type events + /// values are used for PGIDs). + /// caller_id: Internal job id for caller_exit type events union { int signal; - int job_id; + uint64_t caller_id; pid_t pid; } param1{}; diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 5f0de0765..4316e91a8 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -1283,17 +1283,13 @@ eval_result_t parse_execution_context_t::run_1_job(tnode_t job_node, // We are about to populate a job. One possible argument to the job is a command substitution // which may be interested in the job that's populating it, via '--on-job-exit caller'. Record // the job ID here. - auto &libdata = parser->libdata(); - const auto saved_caller_jid = libdata.caller_job_id; - libdata.caller_job_id = job->job_id(); + scoped_push caller_id(&parser->libdata().caller_id, job->internal_job_id); // Populate the job. This may fail for reasons like command_not_found. If this fails, an error // will have been printed. eval_result_t pop_result = this->populate_job_from_job_node(job.get(), job_node, associated_block); - - assert(libdata.caller_job_id == job->job_id() && "Caller job ID unexpectedly changed"); - parser->libdata().caller_job_id = saved_caller_jid; + caller_id.restore(); // Store time it took to 'parse' the command. if (profile_item != nullptr) { diff --git a/src/parser.h b/src/parser.h index 4ae3cc920..ae429ed03 100644 --- a/src/parser.h +++ b/src/parser.h @@ -148,9 +148,9 @@ struct library_data_t { /// Whether we are currently cleaning processes. bool is_cleaning_procs{false}; - /// The job id of the job being populated. + /// The internal job id of the job being populated, or 0 if none. /// This supports the '--on-job-exit caller' feature. - job_id_t caller_job_id{-1}; + internal_job_id_t caller_id{0}; /// Whether we are running a subshell command. bool is_subshell{false}; diff --git a/src/proc.cpp b/src/proc.cpp index c20e543a3..dd8d0cb7f 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -254,9 +254,15 @@ void process_t::check_generations_before_launch() { gens_ = topic_monitor_t::principal().current_generations(); } +static uint64_t next_internal_job_id() { + static std::atomic s_next{}; + 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() { @@ -412,6 +418,7 @@ event_t proc_create_event(const wchar_t *msg, event_type_t type, pid_t pid, int event_t event{type}; event.desc.param1.pid = pid; + event.arguments.reserve(3); event.arguments.push_back(msg); event.arguments.push_back(to_string(pid)); event.arguments.push_back(to_string(status)); @@ -581,7 +588,8 @@ static bool process_clean_after_marking(parser_t &parser, bool allow_interactive proc_create_event(L"JOB_EXIT", event_type_t::exit, -j->pgid, 0)); } exit_events.push_back( - proc_create_event(L"JOB_EXIT", event_type_t::job_exit, j->job_id(), 0)); + proc_create_event(L"JOB_EXIT", event_type_t::caller_exit, j->job_id(), 0)); + exit_events.back().desc.param1.caller_id = j->internal_job_id; } } diff --git a/src/proc.h b/src/proc.h index ea8ab09ec..8ce2276cf 100644 --- a/src/proc.h +++ b/src/proc.h @@ -266,7 +266,12 @@ class process_t { typedef std::unique_ptr process_ptr_t; typedef std::vector process_list_t; -typedef int job_id_t; +/// The non user-visible, never-recycled job ID. +/// Every job has a unique positive value for this. +using internal_job_id_t = uint64_t; + +/// The user-visible, optional, recycled job ID. +using job_id_t = int; job_id_t acquire_job_id(void); void release_job_id(job_id_t jid); @@ -381,8 +386,12 @@ class job_t { pid_t pgid{INVALID_PID}; /// The id of this job. + /// This is user-visible, is recycled, and may be -1. job_id_t job_id() const { return job_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. diff --git a/tests/checks/caller-observer.fish b/tests/checks/caller-observer.fish new file mode 100644 index 000000000..3585ed3a8 --- /dev/null +++ b/tests/checks/caller-observer.fish @@ -0,0 +1,34 @@ +#RUN: %fish %s + +# Verify the '--on-job-exit caller' misfeature. +function make_call_observer -a type + function observer_$type --on-job-exit caller --inherit-variable type + echo "Observed exit of $type" + end +end + +command echo "echo ran 1" (make_call_observer external) +#CHECK: echo ran 1 +#CHECK: Observed exit of external + +builtin echo "echo ran 2" (make_call_observer builtin) +#CHECK: echo ran 2 +#CHECK: Observed exit of builtin + +function func_echo + builtin echo $argv +end + +func_echo "echo ran 3" (make_call_observer function) +#CHECK: echo ran 3 +#CHECK: Observed exit of function + +# They should not fire again. +# TODO: We don't even clean up the functions! + +command echo "echo ran 4" +builtin echo "echo ran 5" +func_echo "echo ran 6" +#CHECK: echo ran 4 +#CHECK: echo ran 5 +#CHECK: echo ran 6