diff --git a/src/exec.cpp b/src/exec.cpp index 0366aebb9..247b3a4fc 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -1002,44 +1002,13 @@ void exec_job(parser_t &parser, shared_ptr j) { return; } - // Unfortunately `exec_job()` is called recursively when functions are encountered, with a new - // job id (and therefore pgrp) each time, but always from the main thread. This breaks terminal - // control since new pgrps take terminal control away from commands upstream in a different pgrp. - // We try to work around this with a heuristic to determine whether to reuse the same pgrp as the - // last-spawned pgrp if part of an existing job pipeline (keeping in mind that new jobs are - // recursively started for both foreground and background jobs, and that a function can expand - // to more than one external command, one (or more?) of which may want to read from upstream or - // write to downstream of a pipe. - // By keeping track of (select) "jobs in flight" we can try to marshall newly-created external - // processes into existing pgrps. Fixes #3952. - // This is a HACK and the correct solution would be to pass the active job through the pipeline - // to the newly established parser context so that the funtion as parsed and evaluated can be - // directly associated with this job and not a new one, BUT sometimes functions NEED to start a - // new job. This HACK seeks a compromise by letting functions trigger the unilateral creation of - // a new job, but reusing the "parent" job's existing pgrp in case of terminal control. - static std::stack active_jobs; - // There's an assumption that there's a one-to-one mapping between jobs under job control and - // pgrps. When we share a parent job's pgrp, we risk reaping its processes before it is fully - // constructed, causing later setpgrp(2) calls to fails (#5219). While the parent job is still - // under construction, child jobs have job_flag_t::WAIT_BY_PROCESS set to prevent early repaing. - // We store them here until the parent job is constructed, at which point it unsets this flag. - static std::stack child_jobs; - - auto parent_job = active_jobs.empty() ? nullptr : active_jobs.top(); - bool job_pushed = false; - if (j->get_flag(job_flag_t::TERMINAL) && j->get_flag(job_flag_t::JOB_CONTROL)) { - // This will be popped before this job leaves exec_job - active_jobs.push(j); - job_pushed = true; - } + const std::shared_ptr parent_job = j->get_parent(); + // Perhaps inherit our parent's pgid and job control flag. if (parent_job && j->processes.front()->type == EXTERNAL) { if (parent_job->pgid != INVALID_PID) { j->pgid = parent_job->pgid; j->set_flag(job_flag_t::JOB_CONTROL, true); - j->set_flag(job_flag_t::NESTED, true); - j->set_flag(job_flag_t::WAIT_BY_PROCESS, true); - child_jobs.push(j); } } @@ -1108,21 +1077,6 @@ void exec_job(parser_t &parser, shared_ptr j) { env_set_one(L"last_pid", ENV_GLOBAL, to_string(j->pgid)); } - if (job_pushed) { - active_jobs.pop(); - } - - if (!parent_job) { - // Unset WAIT_BY_PROCESS on all child jobs. We could leave it, but this speeds up the - // execution of `process_mark_finished_children()`. - while (!child_jobs.empty()) { - auto child = child_jobs.top(); - child_jobs.pop(); - - child->set_flag(job_flag_t::WAIT_BY_PROCESS, false); - } - } - if (!exec_error) { j->continue_job(false); } else { diff --git a/src/proc.cpp b/src/proc.cpp index 6821e5b70..d12562c0b 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -1090,7 +1090,7 @@ void job_t::continue_job(bool send_sigcont) { // is also the only place that send_sigcont is false. parent_job.is_constructed() // must also be true, which coincides with WAIT_BY_PROCESS (which will have to do // since we don't store a reference to the parent job in the job_t structure). - bool block_on_fg = send_sigcont && !get_flag(job_flag_t::WAIT_BY_PROCESS); + bool block_on_fg = send_sigcont && job_chain_is_fully_constructed(); // Wait for data to become available or the status of our own job to change while (!reader_exit_forced() && !is_stopped() && !is_completed()) { diff --git a/src/proc.h b/src/proc.h index 614375f11..d1de80641 100644 --- a/src/proc.h +++ b/src/proc.h @@ -157,13 +157,6 @@ enum class job_flag_t { JOB_CONTROL, /// Whether the job wants to own the terminal when in the foreground. TERMINAL, - /// This job was created via a recursive call to exec_job upon evaluation of a function. - /// Ideally it should not be a top-level job, and there are places where it won't be treated - /// as such. - NESTED, - /// This job shares a pgrp with a not-yet-constructed job, so we can't waitpid on its pgid - /// directly. Hack to work around fucntions starting new jobs. See `exec_job()`. - WAIT_BY_PROCESS }; typedef int job_id_t;