Repository: GuillaumeGomez/sysinfo
Pull request #1436 (2024-12-21 19:33:43)

Introduce ProcessRefreshKind::Thread

GuillaumeGomez: A lot of extra docs to explain what this `thread` is about and that it only affects linux is needed too.
sigsegved: #1432 
GuillaumeGomez: Also: the new setting is still disabled by default.
sigsegved: > Also: the new setting is still disabled by default.
No its not. In default we have `tasks: true,`. If i m not looking at the right thing can you please point me to it.?
sigsegved: @GuillaumeGomez  do you know why this task consistently fails?  https://github.com/GuillaumeGomez/sysinfo/pull/1436/checks?check_run_id=36373553137

Doesnt seem to be related to my code change. its failing in rust setup. 
GuillaumeGomez: > > Also: the new setting is still disabled by default.
> 
> No its not. In default we have `tasks: true,`. If i m not looking at the right thing can you please point me to it.?

Well, don't trust me, trust the code. Try:

```rust
let x = ProcessRefreshKind::new();
assert!(x.tasks());
```

<details>
<summary>Explanations</summary>

It's because you need to implement by hand the `Default` trait.

</details>

Please add a unit test with the code above too please.

> @GuillaumeGomez do you know why this task consistently fails? https://github.com/GuillaumeGomez/sysinfo/pull/1436/checks?check_run_id=36373553137
> 
> Doesnt seem to be related to my code change. its failing in rust setup.

Yes, it's "normal". Rust's freebsd support is not great and there are often random failures depending on the nightly.


sigsegved: > > > Also: the new setting is still disabled by default.
> > 
> > 
> > No its not. In default we have `tasks: true,`. If i m not looking at the right thing can you please point me to it.?
> 
> Well, don't trust me, trust the code. Try:
> 
> ```rust
> let x = ProcessRefreshKind::new();
> assert!(x.tasks());
> ```
> 
> Explanations
> It's because you need to implement by hand the `Default` trait.
> 
> Please add a unit test with the code above too please.
> 

I see what you mean but ProcessRefreshKind does not have a `new()` fn. It has `everything()`  and `nothing()`. `nothing()` uses the default() which means we do want tasks turned off as with other fields. `everything()` on the other hand turns on tasks as expected. I will update the tests to validate that using just everything() gets the tasks list. 

> > @GuillaumeGomez do you know why this task consistently fails? https://github.com/GuillaumeGomez/sysinfo/pull/1436/checks?check_run_id=36373553137
> > Doesnt seem to be related to my code change. its failing in rust setup.
> 
> Yes, it's "normal". Rust's freebsd support is not great and there are often random failures depending on the nightly.
Bummer :( This shouldnt block me from merging this PR thought right?

GuillaumeGomez: > I see what you mean but ProcessRefreshKind does not have a `new()` fn. It has `everything()` and `nothing()`. `nothing()` uses the default() which means we do want tasks turned off as with other fields. `everything()` on the other hand turns on tasks as expected. I will update the tests to validate that using just everything() gets the tasks list.

I actually wanted to write `default()`, but yes, you got the idea.

`nothing` should also get it. It gets the processes list, tasks are processes. So by default I'd still want them to be retrieved.

> Bummer :( This shouldnt block me from merging this PR thought right?

Nah, not an issue. MacOS CI also randomly fails on `test_environ`. Not a blocker, it's a known issue.
sigsegved: @GuillaumeGomez  can i get an approval on this if everything looks good. This is blocking us from going into production in its current state.
GuillaumeGomez: > @GuillaumeGomez can i get an approval on this if everything looks good. This is blocking us from going into production in its current state.

Well, neither you or your company is sponsoring my work so I have to admit it's not really one of my priorities. ^^'

Patch:
diff --git a/src/common/system.rs b/src/common/system.rs
index abb02ac..0c71f7a 100644
--- a/src/common/system.rs
+++ b/src/common/system.rs
@@ -263,7 +263,7 @@ impl System {
         self.inner.refresh_cpu_specifics(refresh_kind)
     }
 
-    /// Gets all processes and updates their information.
+    /// Gets all processes and updates their information, along with all the tasks each process has.
     ///
     /// It does the same as:
     ///
@@ -277,7 +277,7 @@ impl System {
     ///         .with_memory()
     ///         .with_cpu()
     ///         .with_disk_usage()
-    ///         .with_exe(UpdateKind::OnlyIfNotSet),
+    ///         .with_exe(UpdateKind::OnlyIfNotSet)
     /// );
     /// ```
     ///
@@ -288,6 +288,11 @@ impl System {
     /// ⚠️ 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].
     ///
+    /// ⚠️ On Linux, if you dont need the tasks of each process, you can use
+    /// `refresh_processes_specifics` with `ProcessRefreshKind::everything().without_tasks()`.
+    /// Refreshesing all processes and their tasks can be quite expensive. For more information
+    /// see [`ProcessRefreshKind`].
+    ///
     /// Example:
     ///
     /// ```no_run
@@ -308,7 +313,8 @@ impl System {
                 .with_memory()
                 .with_cpu()
                 .with_disk_usage()
-                .with_exe(UpdateKind::OnlyIfNotSet),
+                .with_exe(UpdateKind::OnlyIfNotSet)
+                .with_tasks(),
         )
     }
 
@@ -1868,6 +1874,15 @@ pub enum ProcessesToUpdate<'a> {
 /// the information won't be retrieved if the information is accessible without needing
 /// extra computation.
 ///
+/// ⚠️ ** Linux Specific ** ⚠️
+/// When using `ProcessRefreshKind::everything()`, in linux we will fetch all relevant
+/// information from `/proc/<pid>/` as well as all the information from `/proc/<pid>/task/<tid>/`
+/// folders. This makes the refresh mechanism a lot slower depending on the number of tasks
+/// each process has.
+///  
+/// If you don't care about tasks information, use `ProcessRefreshKind::everything().without_tasks()`
+/// as much as possible.
+///
 /// ```
 /// use sysinfo::{ProcessesToUpdate, ProcessRefreshKind, System};
 ///
@@ -1887,7 +1902,7 @@ pub enum ProcessesToUpdate<'a> {
 /// ```
 ///
 /// [`Process`]: crate::Process
-#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)]
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub struct ProcessRefreshKind {
     cpu: bool,
     disk_usage: bool,
@@ -1898,11 +1913,35 @@ pub struct ProcessRefreshKind {
     environ: UpdateKind,
     cmd: UpdateKind,
     exe: UpdateKind,
+    tasks: bool,
+}
+
+/// Creates a new `ProcessRefreshKind` with every refresh set to `false`, except for `tasks`.
+/// By default, we want to list all processes and tasks are considered processes on their own
+/// in linux so we still fetch them by default. However, the processes information are not
+/// refreshed.
+impl Default for ProcessRefreshKind {
+    fn default() -> Self {
+        Self {
+            cpu: false,
+            disk_usage: false,
+            memory: false,
+            user: UpdateKind::default(),
+            cwd: UpdateKind::default(),
+            root: UpdateKind::default(),
+            environ: UpdateKind::default(),
+            cmd: UpdateKind::default(),
+            exe: UpdateKind::default(),
+            tasks: true, // Process by default includes all tasks.
+        }
+    }
 }
 
 impl ProcessRefreshKind {
-    /// Creates a new `ProcessRefreshKind` with every refresh set to `false`.
-    ///
+    /// Creates a new `ProcessRefreshKind` with every refresh set to `false`, except for `tasks`.
+    /// By default, we want to list all processes and tasks are considered processes on their own
+    /// in linux so we still fetch them by default. However, the processes information are not
+    /// refreshed.
     /// ```
     /// use sysinfo::{ProcessRefreshKind, UpdateKind};
     ///
@@ -1937,6 +1976,7 @@ impl ProcessRefreshKind {
             environ: UpdateKind::OnlyIfNotSet,
             cmd: UpdateKind::OnlyIfNotSet,
             exe: UpdateKind::OnlyIfNotSet,
+            tasks: true,
         }
     }
 
@@ -1986,6 +2026,7 @@ It will retrieve the following information:
     );
     impl_get_set!(ProcessRefreshKind, cmd, with_cmd, without_cmd, UpdateKind);
     impl_get_set!(ProcessRefreshKind, exe, with_exe, without_exe, UpdateKind);
+    impl_get_set!(ProcessRefreshKind, tasks, with_tasks, without_tasks);
 }
 
 /// Used to determine what you want to refresh specifically on the [`Cpu`] type.
diff --git a/src/unix/apple/disk.rs b/src/unix/apple/disk.rs
index 2219494..1fbf17a 100644
--- a/src/unix/apple/disk.rs
+++ b/src/unix/apple/disk.rs
@@ -306,6 +306,8 @@ unsafe fn get_list(container: &mut Vec<Disk>, refresh_kind: DiskRefreshKind) {
 
         let disk = container
             .iter_mut()
+            // FIXME: Using the mount point might not be enough to ensure this disk is the one
+            // we're looking for.
             .find(|d| d.inner.mount_point == mount_point);
         if let Some(disk) = new_disk(
             disk,
diff --git a/src/unix/freebsd/disk.rs b/src/unix/freebsd/disk.rs
index 4aff58e..1100dca 100644
--- a/src/unix/freebsd/disk.rs
+++ b/src/unix/freebsd/disk.rs
@@ -375,7 +375,15 @@ pub unsafe fn get_all_list(
             OsString::from(mount_point)
         };
 
-        if let Some(disk) = container.iter_mut().find(|d| d.inner.name == name) {
+        if let Some(disk) = container.iter_mut().find(|d| {
+            d.inner.name == name
+                && d.inner
+                    .file_system
+                    .as_encoded_bytes()
+                    .iter()
+                    .zip(fs_type.iter())
+                    .all(|(a, b)| a == b)
+        }) {
             // I/O usage is updated for all disks at once at the end.
             refresh_disk(&mut disk.inner, refresh_kind.without_io_usage());
             disk.inner.updated = true;
diff --git a/src/unix/linux/disk.rs b/src/unix/linux/disk.rs
index babbd08..f7379ef 100644
--- a/src/unix/linux/disk.rs
+++ b/src/unix/linux/disk.rs
@@ -406,10 +406,11 @@ fn get_all_list(container: &mut Vec<Disk>, content: &str, refresh_kind: DiskRefr
         })
     {
         let mount_point = Path::new(&fs_file);
-        if let Some(disk) = container
-            .iter_mut()
-            .find(|d| d.inner.mount_point == mount_point)
-        {
+        if let Some(disk) = container.iter_mut().find(|d| {
+            d.inner.mount_point == mount_point
+                && d.inner.device_name == fs_spec
+                && d.inner.file_system == fs_vfstype
+        }) {
             disk.inner
                 .efficient_refresh(refresh_kind, &procfs_disk_stats, false);
             disk.inner.updated = true;
@@ -465,9 +466,9 @@ impl DiskStat {
         // 3rd field
         let name = iter.nth(2).map(ToString::to_string)?;
         // 6th field
-        let sectors_read = iter.nth(2).and_then(|v| u64::from_str(v).ok())?;
+        let sectors_read = iter.nth(2).and_then(|v| u64::from_str(v).ok()).unwrap_or(0);
         // 10th field
-        let sectors_written = iter.nth(3).and_then(|v| u64::from_str(v).ok())?;
+        let sectors_written = iter.nth(3).and_then(|v| u64::from_str(v).ok()).unwrap_or(0);
         Some((
             name,
             Self {
@@ -583,4 +584,32 @@ mod test {
 
         assert_eq!(data, expected_data);
     }
+
+    #[test]
+    fn disk_entry_with_less_information() {
+        let file_content = "\
+ systemd-1      /efi autofs rw,relatime,fd=181,pgrp=1,timeout=120,minproto=5,maxproto=5,direct,pipe_ino=8311 0 0
+ /dev/nvme0n1p1 /efi vfat   rw,nosuid,nodev,noexec,relatime,nosymfollow,fmask=0077,dmask=0077                0 0
+";
+
+        let data = disk_stats_inner(file_content);
+        let expected_data: HashMap<String, DiskStat> = HashMap::from([
+            (
+                "autofs".to_string(),
+                DiskStat {
+                    sectors_read: 0,
+                    sectors_written: 0,
+                },
+            ),
+            (
+                "vfat".to_string(),
+                DiskStat {
+                    sectors_read: 0,
+                    sectors_written: 0,
+                },
+            ),
+        ]);
+
+        assert_eq!(data, expected_data);
+    }
 }
diff --git a/src/unix/linux/process.rs b/src/unix/linux/process.rs
index 310802b..f6d53ef 100644
--- a/src/unix/linux/process.rs
+++ b/src/unix/linux/process.rs
@@ -664,6 +664,7 @@ fn get_all_pid_entries(
     parent_pid: Option<Pid>,
     entry: DirEntry,
     data: &mut Vec<ProcAndTasks>,
+    enable_task_stats: bool,
 ) -> Option<Pid> {
     let Ok(file_type) = entry.file_type() else {
         return None;
@@ -682,17 +683,19 @@ fn get_all_pid_entries(
     let name = name?;
     let pid = Pid::from(usize::from_str(name.to_str()?).ok()?);
 
-    let tasks_dir = Path::join(&entry, "task");
-
-    let tasks = if let Ok(entries) = fs::read_dir(tasks_dir) {
-        let mut tasks = HashSet::new();
-        for task in entries
-            .into_iter()
-            .filter_map(|entry| get_all_pid_entries(Some(name), Some(pid), entry.ok()?, data))
-        {
-            tasks.insert(task);
+    let tasks = if enable_task_stats {
+        let tasks_dir = Path::join(&entry, "task");
+        if let Ok(entries) = fs::read_dir(tasks_dir) {
+            let mut tasks = HashSet::new();
+            for task in entries.into_iter().filter_map(|entry| {
+                get_all_pid_entries(Some(name), Some(pid), entry.ok()?, data, enable_task_stats)
+            }) {
+                tasks.insert(task);
+            }
+            Some(tasks)
+        } else {
+            None
         }
-        Some(tasks)
     } else {
         None
     };
@@ -777,7 +780,7 @@ pub(crate) fn refresh_procs(
             .map(|entry| {
                 let Ok(entry) = entry else { return Vec::new() };
                 let mut entries = Vec::new();
-                get_all_pid_entries(None, None, entry, &mut entries);
+                get_all_pid_entries(None, None, entry, &mut entries, refresh_kind.tasks());
                 entries
             })
             .flatten()
diff --git a/src/windows/disk.rs b/src/windows/disk.rs
index 7fe11ef..1a443e6 100644
--- a/src/windows/disk.rs
+++ b/src/windows/disk.rs
@@ -320,7 +320,10 @@ pub(crate) unsafe fn get_list(
         let name = os_string_from_zero_terminated(&name);
         let file_system = os_string_from_zero_terminated(&file_system);
         for mount_path in mount_paths {
-            if let Some(disk) = disks.iter_mut().find(|d| d.inner.mount_point == mount_path) {
+            if let Some(disk) = disks
+                .iter_mut()
+                .find(|d| d.inner.mount_point == mount_path && d.inner.file_system == file_system)
+            {
                 disk.refresh_specifics(refreshes);
                 disk.inner.updated = true;
                 continue;


Test patch:
diff --git a/tests/process.rs b/tests/process.rs
index e2e8d61..9ff8b33 100644
--- a/tests/process.rs
+++ b/tests/process.rs
@@ -416,55 +416,115 @@ fn test_refresh_process_doesnt_remove() {
     not(feature = "unknown-ci")
 ))]
 fn test_refresh_tasks() {
+    // Skip if unsupported.
     if !sysinfo::IS_SUPPORTED_SYSTEM || cfg!(feature = "apple-sandbox") {
         return;
     }
-    let task_name = "task_1_second";
+
+    // 1) Spawn a thread that waits on a channel, so we control when it exits.
+    let task_name = "controlled_test_thread";
+    let (tx, rx) = std::sync::mpsc::channel::<()>();
+
     std::thread::Builder::new()
-        .name(task_name.into())
-        .spawn(|| {
-            std::thread::sleep(std::time::Duration::from_secs(1));
+        .name(task_name.to_string())
+        .spawn(move || {
+            // Wait until the main thread signals we can exit.
+            let _ = rx.recv();
         })
         .unwrap();
 
     let pid = Pid::from_u32(std::process::id() as _);
+    let mut sys = System::new();
+
+    // Wait until the new thread shows up in the process/tasks list.
+    // We do a short loop and check each time by refreshing processes.
+    const MAX_POLLS: usize = 20;
+    const POLL_INTERVAL: std::time::Duration = std::time::Duration::from_millis(100);
+
+    for _ in 0..MAX_POLLS {
+        sys.refresh_processes(ProcessesToUpdate::All, /*refresh_users=*/ false);
+
+        // Check if our thread is present in two ways:
+        //   (a) via parent's tasks
+        //   (b) by exact name
+        let parent_proc = sys.process(pid);
+        let tasks_contain_thread = parent_proc
+            .and_then(|p| p.tasks())
+            .map(|tids| {
+                tids.iter().any(|tid| {
+                    sys.process(*tid)
+                        .map(|t| t.name() == task_name)
+                        .unwrap_or(false)
+                })
+            })
+            .unwrap_or(false);
+
+        let by_exact_name_exists = sys
+            .processes_by_exact_name(task_name.as_ref())
+            .next()
+            .is_some();
+
+        if tasks_contain_thread && by_exact_name_exists {
+            // We confirmed the thread is now visible
+            break;
+        }
+        std::thread::sleep(POLL_INTERVAL);
+    }
 
-    // Checks that the task is listed as it should.
-    let mut s = System::new();
-    s.refresh_processes(ProcessesToUpdate::All, false);
-
-    assert!(s
-        .process(pid)
-        .unwrap()
-        .tasks()
-        .map(|tasks| tasks.iter().any(|task_pid| s
-            .process(*task_pid)
-            .map(|task| task.name() == task_name)
-            .unwrap_or(false)))
-        .unwrap_or(false));
-    assert!(s
-        .processes_by_exact_name(task_name.as_ref())
-        .next()
-        .is_some());
-
-    // Let's give some time to the system to clean up...
-    std::thread::sleep(std::time::Duration::from_secs(2));
-
-    s.refresh_processes(ProcessesToUpdate::All, true);
+    // At this point we know the task is visible in the system's process/tasks list.
+    // Let's validate a few more things:
+    // * ProcessRefreshKind::nothing() should have task information.
+    // * ProcessRefreshKind::nothing().with_tasks() should have task information.
+    // * ProcessRefreshKind::nothing().without_tasks() shouldn't have task information.
+    // * ProcessRefreshKind::everything() should have task information.
+    // * ProcessRefreshKind::everything() should have task information.
+    // * ProcessRefreshKind::everything().without_tasks() should not have task information.
+
+    let expectations = [
+        (ProcessRefreshKind::nothing(), true),
+        (ProcessRefreshKind::nothing().with_tasks(), true),
+        (ProcessRefreshKind::nothing().without_tasks(), false),
+        (ProcessRefreshKind::everything(), true),
+        (ProcessRefreshKind::everything().with_tasks(), true),
+        (ProcessRefreshKind::everything().without_tasks(), false),
+    ];
+    for (kind, expect_tasks) in expectations.iter() {
+        let mut sys_new = System::new();
+        sys_new.refresh_processes_specifics(ProcessesToUpdate::All, true, *kind);
+        let proc = sys_new.process(pid).unwrap();
+        assert_eq!(proc.tasks().is_some(), *expect_tasks);
+    }
 
-    assert!(!s
-        .process(pid)
-        .unwrap()
-        .tasks()
-        .map(|tasks| tasks.iter().any(|task_pid| s
-            .process(*task_pid)
-            .map(|task| task.name() == task_name)
-            .unwrap_or(false)))
-        .unwrap_or(false));
-    assert!(s
-        .processes_by_exact_name(task_name.as_ref())
-        .next()
-        .is_none());
+    // 3) Signal the thread to exit.
+    drop(tx);
+
+    // 4) Wait until the thread is gone from the system’s process/tasks list.
+    for _ in 0..MAX_POLLS {
+        sys.refresh_processes(ProcessesToUpdate::All, /*refresh_users=*/ true);
+
+        let parent_proc = sys.process(pid as sysinfo::Pid);
+        let tasks_contain_thread = parent_proc
+            .and_then(|p| p.tasks())
+            .map(|tids| {
+                tids.iter().any(|tid| {
+                    sys.process(*tid)
+                        .map(|t| t.name() == task_name)
+                        .unwrap_or(false)
+                })
+            })
+            .unwrap_or(false);
+
+        let by_exact_name_exists = sys
+            .processes_by_exact_name(task_name.as_ref())
+            .next()
+            .is_some();
+
+        // If it's gone from both checks, we're good.
+        if !tasks_contain_thread && !by_exact_name_exists {
+            break;
+        }
+        std::thread::sleep(POLL_INTERVAL);
+    }
 }
 
 // Checks that `refresh_process` is removing dead processes when asked.