diff --git a/src/builtin_function.cpp b/src/builtin_function.cpp index 42b0a4cd0..31a22ce25 100644 --- a/src/builtin_function.cpp +++ b/src/builtin_function.cpp @@ -103,17 +103,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::caller_exit; - e.param1.job_id = job_id; + e.param1.caller_id = caller_id; } else if ((opt == 'p') && (wcscasecmp(w.woptarg, L"%self") == 0)) { e.type = event_type_t::exit; e.param1.pid = getpid(); diff --git a/src/event.cpp b/src/event.cpp index b008d3287..c81472420 100644 --- a/src/event.cpp +++ b/src/event.cpp @@ -114,7 +114,7 @@ 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::caller_exit: { - return classv.desc.param1.job_id == instance.desc.param1.job_id; + 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; @@ -378,7 +378,7 @@ void event_print(io_streams_t &streams, maybe_t type_filter) { case event_type_t::exit: return d1.param1.pid < d2.param1.pid; case event_type_t::caller_exit: - return d1.param1.job_id < d2.param1.job_id; + return d1.param1.caller_id < d2.param1.caller_id; case event_type_t::variable: case event_type_t::any: case event_type_t::generic: diff --git a/src/event.h b/src/event.h index ff48b4d4d..d584358cf 100644 --- a/src/event.h +++ b/src/event.h @@ -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 2bdec06a2..fe7c6a33a 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -1283,17 +1283,13 @@ end_execution_reason_t parse_execution_context_t::run_1_job(tnode_t job_ // 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. end_execution_reason_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 425144df8..bcea710fb 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 3e7963a9b..ce460b6e8 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -275,9 +275,15 @@ bool process_t::is_internal() const { return true; } +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() { @@ -447,6 +453,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)); @@ -617,6 +624,7 @@ static bool process_clean_after_marking(parser_t &parser, bool allow_interactive } exit_events.push_back( 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 7cd54db43..7bf5b1cda 100644 --- a/src/proc.h +++ b/src/proc.h @@ -269,7 +269,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); @@ -411,8 +416,12 @@ class job_t { pgroup_provenance_t pgroup_provenance{}; /// 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