Repository: GuillaumeGomez/sysinfo (language: rust)

Issue:
Refreshing multiple processes on macos leads to invalid cpu usage
I have a piece of code that looks like this:

```rust
let mut sys = sysinfo::System::new();
let process_refresh_kind = ProcessRefreshKind::everything().without_disk_usage().without_environ();
sys.refresh_process_specifics(pid_a, process_refresh_kind);
sys.refresh_process_specifics(pid_b, process_refresh_kind);

loop {
    // Wait 1 minute
    std::thread::sleep(std::time::Duration::from_secs(60));

    // Refresh CPU measurement
    sys.refresh_process_specifics(pid_a, process_refresh_kind);
    sys.refresh_process_specifics(pid_b, process_refresh_kind);

    // Get CPU usage of the two processes
    let cpu_a = sys.process(pid_a).map(|p| p.cpu_usage());
    let cpu_b = sys.process(pid_b).map(|p| p.cpu_usage());
    println!("cpu a: {:?}, cpu b: {:?}", cpu_a, cpu_b);
}
```

With this code, the cpu for process A seems to be valid, but the one for process B is way too high, it can even get higher than the max value.

Looking at the code, I see that the function `compute_cpu_usage` uses a `time_interval` variable. This value seems to be right on the first call to refresh (for pid A), but not on the second call (for pid B). I suppose this value is reset after each call to "refresh_process", leading to the second call computing a CPU load with a time interval that is way too small.

Here is a reproducer:

```rust
fn main() {
    let mut sys = sysinfo::System::new();

    let pid_a = sysinfo::Pid::from_u32(std::process::id());
    let pid_b = sysinfo::Pid::from_u32(1);

    let process_refresh_kind = sysinfo::ProcessRefreshKind::everything()
        .without_disk_usage()
        .without_environ();
    sys.refresh_process_specifics(pid_a, process_refresh_kind);
    sys.refresh_process_specifics(pid_b, process_refresh_kind);

    loop {
        std::thread::sleep(std::time::Duration::from_secs(10));

        sys.refresh_process_specifics(pid_a, process_refresh_kind);
        sys.refresh_process_specifics(pid_b, process_refresh_kind);

        let cpu_a = sys.process(pid_a).map(|p| p.cpu_usage());
        let cpu_b = sys.process(pid_b).map(|p| p.cpu_usage());
        println!("cpu a: {:?}, cpu b: {:?}", cpu_a, cpu_b);
    }
}
```

Reversing the two PIDs reverses the one that is reported with a very high CPU usage, showing the bug.

This was reproduced on 0.30.12 on macos x64.


Discussion / hints:
For this kind of operations, you likely want to use [refresh_pids_specifics](https://docs.rs/sysinfo/latest/sysinfo/struct.System.html#method.refresh_pids_specifics). Although, in this case it should still work.
Indeed, I didn't know about this API, and it fixes the issue. I'm leaving this issue open since there is still a bug afaict, but i'm no longer blocked by it, I let you decide what to do with it. Thanks!
It definitely needs to be fixed. Gonna try to take a look in the next days.

Fix patch:
diff --git a/src/common/system.rs b/src/common/system.rs
index 219d9881d..5fd0d0548 100644
--- a/src/common/system.rs
+++ b/src/common/system.rs
@@ -367,6 +367,9 @@ impl System {
     /// exist (it will **NOT** be removed from the processes if it doesn't exist anymore). If it
     /// isn't listed yet, it'll be added.
     ///
+    /// ⚠️ If you need to refresh multiple processes at once, use [`refresh_pids`] instead! It has
+    /// much better performance.
+    ///
     /// It is the same as calling:
     ///
     /// ```no_run
@@ -394,6 +397,8 @@ impl System {
     /// let mut s = System::new_all();
     /// s.refresh_process(Pid::from(1337));
     /// ```
+    ///
+    /// [`refresh_pids`]: #method.refresh_pids
     pub fn refresh_process(&mut self, pid: Pid) -> bool {
         self.refresh_process_specifics(
             pid,
@@ -409,6 +414,9 @@ impl System {
     /// exist (it will **NOT** be removed from the processes if it doesn't exist anymore). If it
     /// isn't listed yet, it'll be added.
     ///
+    /// ⚠️ If you need to refresh multiple processes at once, use [`refresh_pids_specifics`]
+    /// instead! It has much better performance.
+    ///
     /// ⚠️ On Linux, `sysinfo` keeps the `stat` files open by default. You can change this behaviour
     /// by using [`set_open_files_limit`][crate::set_open_files_limit].
     ///
@@ -418,6 +426,8 @@ impl System {
     /// let mut s = System::new_all();
     /// s.refresh_process_specifics(Pid::from(1337), ProcessRefreshKind::new());
     /// ```
+    ///
+    /// [`refresh_pids_specifics`]: #method.refresh_pids_specifics
     pub fn refresh_process_specifics(
         &mut self,
         pid: Pid,
diff --git a/src/unix/apple/macos/system.rs b/src/unix/apple/macos/system.rs
index d0191eb2e..6c182ae1f 100644
--- a/src/unix/apple/macos/system.rs
+++ b/src/unix/apple/macos/system.rs
@@ -8,6 +8,7 @@ use libc::{
     processor_cpu_load_info_t, sysconf, vm_page_size, PROCESSOR_CPU_LOAD_INFO, _SC_CLK_TCK,
 };
 use std::ptr::null_mut;
+use std::time::Instant;
 
 struct ProcessorCpuLoadInfo {
     cpu_load: processor_cpu_load_info_t,
@@ -55,6 +56,8 @@ pub(crate) struct SystemTimeInfo {
     timebase_to_ns: f64,
     clock_per_sec: f64,
     old_cpu_info: ProcessorCpuLoadInfo,
+    last_update: Option<Instant>,
+    prev_time_interval: f64,
 }
 
 unsafe impl Send for SystemTimeInfo {}
@@ -97,40 +100,52 @@ impl SystemTimeInfo {
                 timebase_to_ns: info.numer as f64 / info.denom as f64,
                 clock_per_sec: nano_per_seconds / clock_ticks_per_sec as f64,
                 old_cpu_info,
+                last_update: None,
+                prev_time_interval: 0.,
             })
         }
     }
 
     pub fn get_time_interval(&mut self, port: mach_port_t) -> f64 {
-        let mut total = 0;
-        let new_cpu_info = match ProcessorCpuLoadInfo::new(port) {
-            Some(cpu_info) => cpu_info,
-            None => return 0.,
-        };
-        let cpu_count = std::cmp::min(self.old_cpu_info.cpu_count, new_cpu_info.cpu_count);
-        unsafe {
-            for i in 0..cpu_count {
-                let new_load: &processor_cpu_load_info = &*new_cpu_info.cpu_load.offset(i as _);
-                let old_load: &processor_cpu_load_info =
-                    &*self.old_cpu_info.cpu_load.offset(i as _);
-                for (new, old) in new_load.cpu_ticks.iter().zip(old_load.cpu_ticks.iter()) {
-                    if new > old {
-                        total += new.saturating_sub(*old);
+        let need_cpu_usage_update = self
+            .last_update
+            .map(|last_update| last_update.elapsed() > crate::MINIMUM_CPU_UPDATE_INTERVAL)
+            .unwrap_or(true);
+        if need_cpu_usage_update {
+            let mut total = 0;
+            let new_cpu_info = match ProcessorCpuLoadInfo::new(port) {
+                Some(cpu_info) => cpu_info,
+                None => return 0.,
+            };
+            let cpu_count = std::cmp::min(self.old_cpu_info.cpu_count, new_cpu_info.cpu_count);
+            unsafe {
+                for i in 0..cpu_count {
+                    let new_load: &processor_cpu_load_info = &*new_cpu_info.cpu_load.offset(i as _);
+                    let old_load: &processor_cpu_load_info =
+                        &*self.old_cpu_info.cpu_load.offset(i as _);
+                    for (new, old) in new_load.cpu_ticks.iter().zip(old_load.cpu_ticks.iter()) {
+                        if new > old {
+                            total += new.saturating_sub(*old);
+                        }
                     }
                 }
             }
 
             self.old_cpu_info = new_cpu_info;
+            self.last_update = Some(Instant::now());
 
             // Now we convert the ticks to nanoseconds (if the interval is less than
             // `MINIMUM_CPU_UPDATE_INTERVAL`, we replace it with it instead):
             let base_interval = total as f64 / cpu_count as f64 * self.clock_per_sec;
             let smallest = crate::MINIMUM_CPU_UPDATE_INTERVAL.as_secs_f64() * 1_000_000_000.0;
-            if base_interval < smallest {
+            self.prev_time_interval = if base_interval < smallest {
                 smallest
             } else {
                 base_interval / self.timebase_to_ns
-            }
+            };
+            self.prev_time_interval
+        } else {
+            self.prev_time_interval
         }
     }
 }


Test patch:
diff --git a/tests/process.rs b/tests/process.rs
index f6f841a6f..e0963de56 100644
--- a/tests/process.rs
+++ b/tests/process.rs
@@ -813,3 +813,47 @@ fn test_parent_change() {
     // We kill the child to clean up.
     child.kill();
 }
+
+// We want to ensure that if `System::refresh_process*` methods are called
+// one after the other, it won't impact the CPU usage computation badly.
+#[test]
+fn test_multiple_single_process_refresh() {
+    if !sysinfo::IS_SUPPORTED_SYSTEM || cfg!(feature = "apple-sandbox") || cfg!(windows) {
+        // Windows never updates its parent PID so no need to check anything.
+        return;
+    }
+
+    let file_name = "target/test_binary3";
+    build_test_binary(file_name);
+    let mut p_a = std::process::Command::new(format!("./{file_name}"))
+        .arg("1")
+        .spawn()
+        .unwrap();
+    let mut p_b = std::process::Command::new(format!("./{file_name}"))
+        .arg("1")
+        .spawn()
+        .unwrap();
+
+    let pid_a = Pid::from_u32(p_a.id() as _);
+    let pid_b = Pid::from_u32(p_b.id() as _);
+
+    let mut s = System::new();
+    let process_refresh_kind = ProcessRefreshKind::new().with_cpu();
+    s.refresh_process_specifics(pid_a, process_refresh_kind);
+    s.refresh_process_specifics(pid_b, process_refresh_kind);
+
+    std::thread::sleep(std::time::Duration::from_secs(1));
+    s.refresh_process_specifics(pid_a, process_refresh_kind);
+    s.refresh_process_specifics(pid_b, process_refresh_kind);
+
+    let cpu_a = s.process(pid_a).unwrap().cpu_usage();
+    let cpu_b = s.process(pid_b).unwrap().cpu_usage();
+
+    p_a.kill().expect("failed to kill process a");
+    p_b.kill().expect("failed to kill process b");
+
+    let _ = p_a.wait();
+    let _ = p_b.wait();
+
+    assert!(cpu_b - 5. < cpu_a && cpu_b + 5. > cpu_a);
+}