From ef2e30cdc18efa6fd7ff7a80563698220322a054 Mon Sep 17 00:00:00 2001 From: Peter Ammon Date: Sun, 1 Jun 2025 17:28:38 -0700 Subject: [PATCH] Clean up pids Use OnceLock more often in place of atomics. Tighten up async signal safety. --- src/builtins/jobs.rs | 4 ++-- src/exec.rs | 8 ++++++-- src/job_group.rs | 17 +++++++---------- src/parser.rs | 3 ++- src/proc.rs | 40 ++++++++++------------------------------ 5 files changed, 27 insertions(+), 45 deletions(-) diff --git a/src/builtins/jobs.rs b/src/builtins/jobs.rs index 3088e06e6..cbbfa1f44 100644 --- a/src/builtins/jobs.rs +++ b/src/builtins/jobs.rs @@ -32,7 +32,7 @@ fn cpu_use(j: &Job) -> f64 { let mut u = 0.0; for p in j.external_procs() { let now = timef(); - let jiffies = proc_get_jiffies(p.pid.load().unwrap()); + let jiffies = proc_get_jiffies(*p.pid.get().unwrap()); let last_jiffies = p.last_times.get().jiffies; let since = now - last_jiffies as f64; if since > 0.0 && jiffies > last_jiffies { @@ -99,7 +99,7 @@ fn builtin_jobs_print(j: &Job, mode: JobsPrintMode, header: bool, streams: &mut } for p in j.external_procs() { - out += &sprintf!("%d\n", p.pid.load().unwrap())[..]; + out += &sprintf!("%d\n", *p.pid.get().unwrap())[..]; } streams.out.append(out); } diff --git a/src/exec.rs b/src/exec.rs index 2102adf33..ca7dc9ac7 100644 --- a/src/exec.rs +++ b/src/exec.rs @@ -706,11 +706,15 @@ fn fork_child_for_process( if pid < 0 { return Err(()); } + + // Note we are now post-fork if `is_parent is false. + // ONLY ASYNC SIGNAL-SAFE FUNCTIONS MAY BE CALLED AFTER FORK. let is_parent = pid > 0; // Record the pgroup if this is the leader. // Both parent and child attempt to send the process to its new group, to resolve the race. let pid = if is_parent { pid } else { crate::nix::getpid() }; + let pid: Pid = Pid::new(pid).unwrap(); p.set_pid(pid); if p.leads_pgrp { job.group().set_pgid(pid); @@ -886,9 +890,9 @@ fn exec_external_command( ); // these are all things do_fork() takes care of normally (for forked processes): - p.pid.store(pid); + p.set_pid(pid); if p.leads_pgrp { - j.group().set_pgid(pid.as_pid_t()); + j.group().set_pgid(pid); // posix_spawn should in principle set the pgid before returning. // In glibc, posix_spawn uses fork() and the pgid group is set on the child side; // therefore the parent may not have seen it be set yet. diff --git a/src/job_group.rs b/src/job_group.rs index 484379812..2dacab028 100644 --- a/src/job_group.rs +++ b/src/job_group.rs @@ -1,11 +1,11 @@ use crate::global_safety::RelaxedAtomicBool; -use crate::proc::{AtomicPid, JobGroupRef, Pid}; +use crate::proc::{JobGroupRef, Pid}; use crate::signal::Signal; use crate::wchar::prelude::*; use std::cell::RefCell; use std::num::NonZeroU32; use std::sync::atomic::{AtomicI32, Ordering}; -use std::sync::{Arc, Mutex}; +use std::sync::{Arc, Mutex, OnceLock}; /// A job id, corresponding to what is printed by `jobs`. 1 is the first valid job id. #[derive(Clone, Copy, Debug, Eq, Ord, PartialEq, PartialOrd)] @@ -73,7 +73,7 @@ pub struct JobGroup { pub is_foreground: RelaxedAtomicBool, /// The pgid leading our group. This is only ever set if [`job_control`](Self::job_control) is /// true. We ensure the value (when set) is always non-negative and non-zero. - pgid: AtomicPid, + pgid: OnceLock, /// The original command which produced this job tree. pub command: WString, /// Our job id, if any. `None` here should evaluate to `-1` for ffi purposes. @@ -143,20 +143,17 @@ pub fn cancel_with_signal(&self, signal: Signal) { /// /// As such, this method takes `&mut self` rather than `&self` to enforce that this operation is /// only available during initial construction/initialization. - pub fn set_pgid(&self, pgid: libc::pid_t) { + pub fn set_pgid(&self, pgid: Pid) { assert!( self.wants_job_control(), "Should not set a pgid for a group that doesn't want job control!" ); - assert!(pgid >= 0, "Invalid pgid!"); - - let old_pgid = self.pgid.swap(Pid::new(pgid).unwrap()); - assert!(old_pgid.is_none(), "JobGroup::pgid already set!"); + self.pgid.set(pgid).expect("JobGroup::pgid already set!"); } /// Returns the value of [`JobGroup::pgid`]. This is never fish's own pgid! pub fn get_pgid(&self) -> Option { - self.pgid.load() + self.pgid.get().copied() } } @@ -223,7 +220,7 @@ pub fn new(command: WString, id: MaybeJobId, job_control: bool, wants_term: bool tmodes: RefCell::default(), signal: 0.into(), is_foreground: RelaxedAtomicBool::new(false), - pgid: AtomicPid::default(), + pgid: OnceLock::new(), } } diff --git a/src/parser.rs b/src/parser.rs index f57cff0d5..a58cefb46 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -1053,10 +1053,11 @@ pub fn job_get_from_pid(&self, pid: Pid) -> Option { } /// Returns the job and job index with the given pid. + /// This assumes that all external jobs have a pid. pub fn job_get_with_index_from_pid(&self, pid: Pid) -> Option<(usize, JobRef)> { for (i, job) in self.jobs().iter().enumerate() { for p in job.external_procs() { - if p.pid.load().unwrap() == pid { + if p.pid().unwrap() == pid { return Some((i, job.clone())); } } diff --git a/src/proc.rs b/src/proc.rs index 752b1960e..f743b0f0d 100644 --- a/src/proc.rs +++ b/src/proc.rs @@ -42,7 +42,7 @@ use std::rc::Rc; #[cfg(target_has_atomic = "64")] use std::sync::atomic::AtomicU64; -use std::sync::atomic::{AtomicU32, AtomicU8, Ordering}; +use std::sync::atomic::{AtomicU8, Ordering}; use std::sync::{Arc, OnceLock}; /// Types of processes. @@ -513,24 +513,6 @@ fn to_arg(self) -> fish_printf::Arg<'static> { } } -#[derive(Default, Debug)] -pub struct AtomicPid(AtomicU32); - -impl AtomicPid { - #[inline(always)] - pub fn load(&self) -> Option { - Pid::new(self.0.load(Ordering::Relaxed) as i32) - } - #[inline(always)] - pub fn store(&self, pid: Pid) { - self.0.store(pid.get() as u32, Ordering::Relaxed); - } - #[inline(always)] - pub fn swap(&self, pid: Pid) -> Option { - Pid::new(self.0.swap(pid.get() as u32, Ordering::Relaxed) as i32) - } -} - /// A structure representing a single fish process. Contains variables for tracking process state /// and the process argument list. Actually, a fish process can be either a regular external /// process, an internal builtin which may or may not spawn a fake IO process during execution, a @@ -571,7 +553,7 @@ pub struct Process { pub gens: GenerationsList, /// Process ID or `None` where not available. - pub pid: AtomicPid, + pub pid: OnceLock, /// If we are an "internal process," that process. pub internal_proc: RefCell>>, @@ -635,7 +617,7 @@ pub fn new() -> Self { /// Retrieves the associated [`libc::pid_t`], `None` if unset. #[inline(always)] pub fn pid(&self) -> Option { - self.pid.load() + self.pid.get().copied() } #[inline(always)] @@ -644,10 +626,10 @@ pub fn has_pid(&self) -> bool { } /// Sets the process' pid. Panics if a pid has already been set. - pub fn set_pid(&self, pid: libc::pid_t) { - let pid = Pid::new(pid).expect("Invalid pid passed to Process::set_pid()"); - let old = self.pid.swap(pid); - assert!(old.is_none(), "Process::set_pid() called more than once!"); + pub fn set_pid(&self, pid: Pid) { + self.pid + .set(pid) + .expect("Process::set_pid() called more than once!"); } /// Sets `argv`. @@ -859,7 +841,7 @@ pub fn processes_mut(&mut self) -> &mut Vec { /// Equivalent to `processes().iter().filter(|p| p.pid.is_some())`. #[inline(always)] pub fn external_procs(&self) -> impl Iterator { - self.processes.iter().filter(|p| p.pid.load().is_some()) + self.processes.iter().filter(|p| p.pid().is_some()) } /// Return whether it is OK to reap a given process. Sometimes we want to defer reaping a @@ -899,9 +881,7 @@ pub fn get_pgid(&self) -> Option { /// This may be none if the job consists of just internal fish functions or builtins. /// This will never be fish's own pid. pub fn get_last_pid(&self) -> Option { - self.external_procs() - .last() - .and_then(|proc| proc.pid.load()) + self.external_procs().last().and_then(|proc| proc.pid()) } /// The id of this job. @@ -1282,7 +1262,7 @@ pub fn proc_update_jiffies(parser: &Parser) { for p in job.external_procs() { p.last_times.replace(ProcTimes { time: timef(), - jiffies: proc_get_jiffies(p.pid.load().unwrap()), + jiffies: proc_get_jiffies(p.pid().unwrap()), }); } }