diff --git a/src/builtin_bg.cpp b/src/builtin_bg.cpp index cf645a508..fa8aa45e7 100644 --- a/src/builtin_bg.cpp +++ b/src/builtin_bg.cpp @@ -30,7 +30,7 @@ static int send_to_bg(parser_t &parser, io_streams_t &streams, job_t *j) { streams.err.append_format(_(L"Send job %d '%ls' to background\n"), j->job_id(), j->command_wcstr()); parser.job_promote(j); - j->mut_flags().foreground = false; + j->group->set_is_foreground(false); j->continue_job(parser, true, j->is_stopped()); return STATUS_CMD_OK; } diff --git a/src/builtin_fg.cpp b/src/builtin_fg.cpp index 3b4dbe5e8..9e1855944 100644 --- a/src/builtin_fg.cpp +++ b/src/builtin_fg.cpp @@ -104,7 +104,7 @@ int builtin_fg(parser_t &parser, io_streams_t &streams, wchar_t **argv) { reader_write_title(job->command(), parser); parser.job_promote(job); - job->mut_flags().foreground = true; + job->group->set_is_foreground(true); job->continue_job(parser, true, job->is_stopped()); return STATUS_CMD_OK; diff --git a/src/exec.cpp b/src/exec.cpp index 45ce0cae1..9ef1a6464 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -136,7 +136,7 @@ static bool can_use_posix_spawn_for_job(const std::shared_ptr &job, if (job->wants_job_control()) { //!OCLINT(collapsible if statements) // We are going to use job control; therefore when we launch this job it will get its own // process group ID. But will it be foregrounded? - if (job->should_claim_terminal()) { + if (job->group->should_claim_terminal()) { // It will be foregrounded, so we will call tcsetpgrp(), therefore do not use // posix_spawn. return false; @@ -321,7 +321,8 @@ static bool fork_child_for_process(const std::shared_ptr &job, process_t if (int err = execute_setpgid(p->pid, pgid, false /* not parent */)) { report_setpgid_error(err, pgid, job.get(), p); } - child_setup_process(job->should_claim_terminal() ? pgid : INVALID_PID, *job, true, dup2s); + child_setup_process(job->group->should_claim_terminal() ? pgid : INVALID_PID, *job, true, + dup2s); child_action(); DIE("Child process returned control to fork_child lambda!"); } diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 52379d2cf..0e08a5f8f 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -992,7 +992,7 @@ end_execution_reason_t parse_execution_context_t::populate_not_process( flags.negate = !flags.negate; if (not_statement.time) { flags.has_time_prefix = true; - if (!job->mut_flags().foreground) { + if (job->is_initially_background()) { return this->report_error(STATUS_INVALID_ARGS, not_statement, ERROR_TIME_BACKGROUND); } } @@ -1313,8 +1313,6 @@ end_execution_reason_t parse_execution_context_t::run_1_job(const ast::job_t &jo shared_ptr job = std::make_shared(props); job->tmodes = tmodes; - job->mut_flags().foreground = !props.initial_background; - // 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. diff --git a/src/proc.cpp b/src/proc.cpp index 19e443eb1..7b6402694 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -241,14 +241,9 @@ void print_exit_warning_for_jobs(const job_list_t &jobs) { fputws(_(L"Use 'disown PID' to remove jobs from the list without terminating them.\n"), stdout); } -job_group_t::job_group_t(bool job_control, bool internal) - : job_control_(job_control), - is_internal_(internal), - job_id_(internal ? -1 : acquire_job_id()) {} - job_group_t::~job_group_t() { - if (job_id_ > 0) { - release_job_id(job_id_); + if (props_.job_id > 0) { + release_job_id(props_.job_id); } } @@ -270,15 +265,16 @@ void job_group_t::populate_tree_for_job(job_t *job, const job_group_ref_t &propo // non-internal -> we are running as part of a real pipeline // Decide if this job can use an internal tree. // This is true if it's a simple foreground execution of an internal proc. + bool initial_bg = job->is_initially_background(); bool first_proc_internal = job->processes.front()->is_internal(); - bool can_use_internal = !job->is_initially_background() && job->processes.size() == 1 && - job->processes.front()->is_internal(); + bool can_use_internal = + !initial_bg && job->processes.size() == 1 && job->processes.front()->is_internal(); bool needs_new_tree = false; if (!proposed) { // We don't have a tree yet. needs_new_tree = true; - } else if (!job->is_foreground()) { + } else if (initial_bg) { // Background jobs always get a new tree. needs_new_tree = true; } else if (proposed->is_internal() && !can_use_internal) { @@ -287,20 +283,25 @@ void job_group_t::populate_tree_for_job(job_t *job, const job_group_ref_t &propo } job->mut_flags().is_tree_root = needs_new_tree; - bool job_control = job->wants_job_control(); if (!needs_new_tree) { job->group = proposed; - } else if (can_use_internal) { - job->group.reset(new job_group_t(job_control, true)); } else { - job->group.reset(new job_group_t(job_control, false)); + properties_t props{}; + props.job_control = job->wants_job_control(); + props.wants_terminal = job->wants_job_control() && !job->from_event_handler(); + props.is_internal = can_use_internal; + props.job_id = can_use_internal ? -1 : acquire_job_id(); + job->group.reset(new job_group_t(props)); + + // Mark if it's foreground. + job->group->set_is_foreground(!initial_bg); // Perhaps this job should immediately live in fish's pgroup. // There's two reasons why it may be so: // 1. The job doesn't need job control. // 2. The first process in the job is internal to fish; this needs to own the tty. - if (!job_control || first_proc_internal) { + if (!can_use_internal && (!props.job_control || first_proc_internal)) { job->group->set_pgid(getpgrp()); } } @@ -497,7 +498,7 @@ static void process_mark_finished_children(parser_t &parser, bool block_ok) { assert(pid == proc->pid && "Unexpcted waitpid() return"); handle_child_status(proc.get(), proc_status_t::from_waitpid(status)); if (proc->status.stopped()) { - j->mut_flags().foreground = false; + j->group->set_is_foreground(false); } if (proc->status.continued()) { j->mut_flags().notified = false; @@ -802,7 +803,7 @@ void proc_update_jiffies(parser_t &parser) { int terminal_maybe_give_to_job(const job_t *j, bool continuing_from_stopped) { enum { notneeded = 0, success = 1, error = -1 }; - if (!j->should_claim_terminal()) { + if (!j->group->should_claim_terminal()) { // The job doesn't want the terminal. return notneeded; } diff --git a/src/proc.h b/src/proc.h index a702547a1..c05154ed7 100644 --- a/src/proc.h +++ b/src/proc.h @@ -188,17 +188,26 @@ class job_group_t { maybe_t get_pgid() const; /// \return whether we want job control - bool wants_job_control() const { return job_control_; } + bool wants_job_control() const { return props_.job_control; } /// \return whether this is an internal group. - bool is_internal() const { return is_internal_; } + bool is_internal() const { return props_.is_internal; } + + /// \return whether we are currently the foreground group. + bool is_foreground() const { return is_foreground_; } + + /// Mark whether we are in the foreground. + void set_is_foreground(bool flag) { is_foreground_ = flag; } + + /// \return if this job group should own the terminal when it runs. + bool should_claim_terminal() const { return props_.wants_terminal && is_foreground(); } /// \return whether this job group is awaiting a pgid. /// This is true for non-internal trees that don't already have a pgid. - bool needs_pgid_assignment() const { return !is_internal_ && !pgid_.has_value(); } + bool needs_pgid_assignment() const { return !props_.is_internal && !pgid_.has_value(); } /// \return the job ID, or -1 if none. - job_id_t get_id() const { return job_id_; } + job_id_t get_id() const { return props_.job_id; } /// Mark the root as constructed. /// This is used to avoid reaping a process group leader while there are still procs that may @@ -213,13 +222,33 @@ class job_group_t { ~job_group_t(); private: + // The pgid to assign to jobs, or none if not yet set. maybe_t pgid_{}; - const bool job_control_; - const bool is_internal_; - const job_id_t job_id_; + + // Set of properties, which are constant. + struct properties_t { + // Whether jobs in this group should have job control. + bool job_control{}; + + // Whether we should claim the terminal when we run in the foreground. + // TODO: this is effectively the same as job control, rationalize this. + bool wants_terminal{}; + + // Whether we are an internal job group. + bool is_internal{}; + + // The job ID of this group. + job_id_t job_id{}; + }; + const properties_t props_; + + // Whether we are in the foreground, meaning that the user is waiting for this. + relaxed_atomic_bool_t is_foreground_{}; + + // Whether the root job is constructed. If not, we cannot reap it yet. relaxed_atomic_bool_t root_constructed_{}; - explicit job_group_t(bool job_control, bool internal); + explicit job_group_t(const properties_t &props) : props_(props) {} }; /// A structure representing a single fish process. Contains variables for tracking process state @@ -474,9 +503,6 @@ class job_t { /// Whether the user has been told about stopped job. bool notified{false}; - /// Whether this job is in the foreground. - bool foreground{false}; - /// Whether the exit status should be negated. This flag can only be set by the not builtin. bool negate{false}; @@ -500,9 +526,6 @@ class job_t { /// \return if we want job control. bool wants_job_control() const { return properties.job_control; } - /// \return if this job should own the terminal when it runs. - bool should_claim_terminal() const { return properties.wants_terminal && is_foreground(); } - /// \return whether this job is initially going to run in the background, because & was /// specified. bool is_initially_background() const { return properties.initial_background; } @@ -519,8 +542,6 @@ class job_t { // Helper functions to check presence of flags on instances of jobs /// The job has been fully constructed, i.e. all its member processes have been launched bool is_constructed() const { return flags().constructed; } - /// The job was launched in the foreground and has control of the terminal - bool is_foreground() const { return flags().foreground; } /// The job is complete, i.e. all its member processes have been reaped bool is_completed() const; /// The job is in a stopped state @@ -532,6 +553,9 @@ class job_t { bool skip_notification() const { return properties.skip_notification; } bool from_event_handler() const { return properties.from_event_handler; } + /// \return whether this job's group is in the foreground. + bool is_foreground() const { return group->is_foreground(); } + /// \return whether we should report process exit events. /// This implements some historical behavior which has not been justified. bool should_report_process_exits() const;