Clean up pids

Use OnceLock more often in place of atomics. Tighten up async signal safety.

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);
         }

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.

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<Pid>,
     /// 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<Pid> {
-        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(),
         }
     }
 

src/parser.rs

@@ -1053,10 +1053,11 @@ pub fn job_get_from_pid(&self, pid: Pid) -> Option<JobRef> {
     }
 
     /// 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()));
                 }
             }

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> {
-        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> {
-        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<Pid>,
 
     /// If we are an "internal process," that process.
     pub internal_proc: RefCell<Option<Arc<InternalProc>>>,
@@ -635,7 +617,7 @@ pub fn new() -> Self {
     /// Retrieves the associated [`libc::pid_t`], `None` if unset.
     #[inline(always)]
     pub fn pid(&self) -> Option<Pid> {
-        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<Process> {
     /// Equivalent to `processes().iter().filter(|p| p.pid.is_some())`.
     #[inline(always)]
     pub fn external_procs(&self) -> impl Iterator<Item = &Process> {
-        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<Pid> {
     /// 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<Pid> {
-        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()),
             });
         }
     }