Introduce ProcessRefreshKind::Thread #1436

sigsegved · 2024-12-21T19:33:43Z

No description provided.

src/common/system.rs

src/unix/linux/process.rs

GuillaumeGomez · 2025-01-15T21:32:15Z

A lot of extra docs to explain what this thread is about and that it only affects linux is needed too.

sigsegved · 2025-01-29T03:24:59Z

#1432

src/common/system.rs

GuillaumeGomez · 2025-01-29T12:46:21Z

Also: the new setting is still disabled by default.

sigsegved · 2025-01-29T18:43:42Z

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 · 2025-01-29T18:50:29Z

@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 · 2025-01-29T19:04:14Z

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:

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.

@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 · 2025-01-29T19:54:33Z

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:
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 · 2025-01-29T19:57:21Z

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 · 2025-01-30T18:39:49Z

@GuillaumeGomez can i get an approval on this if everything looks good. This is blocking us from going into production in its current state.

src/common/system.rs

src/unix/linux/process.rs

tests/process.rs

GuillaumeGomez · 2025-01-30T20:34:42Z

@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. ^^'

Co-authored-by: Guillaume Gomez <[email protected]>

sigsegved · 2025-01-30T22:27:39Z

@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. ^^'

Going through your page, looks like my company was a previous sponsor. DM me and i would like to find out how you got sponsored before and see if I can re-engage the same path. Apologies, I'm super new to OS contributions.

GuillaumeGomez · 2025-01-31T10:55:12Z

src/common/system.rs

+/// dirs. 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_thread()`


without_tasks()

GuillaumeGomez · 2025-01-31T10:55:54Z

tests/process.rs

-
-    s.refresh_processes(ProcessesToUpdate::All, true);
+    // At this point we know the thread is visible in the system's process/tasks list.
+    // Lets validate a few more things:


GuillaumeGomez · 2025-01-31T10:56:25Z

tests/process.rs

+    // Lets validate a few more things:
+    // * ProcessRefreshKind::nothing() should have thread information.
+    // * ProcessRefreshKind::nothing().with_tasks() should have thread information.
+    // * ProcessRefreshKind::nothing().without_tasks() should have thread information.


"shouldn't", "task information"

GuillaumeGomez · 2025-01-31T10:56:29Z

tests/process.rs

+    // At this point we know the thread is visible in the system's process/tasks list.
+    // Lets validate a few more things:
+    // * ProcessRefreshKind::nothing() should have thread information.
+    // * ProcessRefreshKind::nothing().with_tasks() should have thread information.


"task information"

GuillaumeGomez · 2025-01-31T10:56:44Z

tests/process.rs

+    // * ProcessRefreshKind::nothing().with_tasks() should have thread information.
+    // * ProcessRefreshKind::nothing().without_tasks() should have thread information.
+    // * ProcessRefreshKind::everything() should have thread information.
+    // * ProcessRefreshKind::everything() should have thread information.


It's the same line as before.

GuillaumeGomez · 2025-01-31T10:56:49Z

tests/process.rs

+    // * ProcessRefreshKind::nothing() should have thread information.
+    // * ProcessRefreshKind::nothing().with_tasks() should have thread information.
+    // * ProcessRefreshKind::nothing().without_tasks() should have thread information.
+    // * ProcessRefreshKind::everything() should have thread information.


"task information"

GuillaumeGomez · 2025-01-31T10:59:14Z

src/common/system.rs

+    /// Creates a new `ProcessRefreshKind` with every refresh set to `false`, except for `tasks`
+    /// This is because by default, [`Process`] was fetching all tasks and we want to keep this
+    /// behavior.


Suggested change

/// Creates a new `ProcessRefreshKind` with every refresh set to `false`, except for `tasks`

/// This is because by default, [`Process`] was fetching all tasks and we want to keep this

/// behavior.

/// 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.

GuillaumeGomez · 2025-01-31T10:59:26Z

src/common/system.rs

+/// Default implementation for [`ProcessRefreshKind`].
+/// Sets everything to default values except for `tasks` which is set to `true`.
+/// This is because by default, a [`Process`] was fetching all tasks and we want to keep this
+/// behavior.


Suggested change

/// Default implementation for [`ProcessRefreshKind`].

/// Sets everything to default values except for `tasks` which is set to `true`.

/// This is because by default, a [`Process`] was fetching all tasks and we want to keep this

/// behavior.

/// 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.

GuillaumeGomez · 2025-01-31T10:59:53Z

I think this is the last review round before merging (if I didn't forget anything hopefully).

Introduce ProcessRefreshKind::Thread

435f428

GuillaumeGomez reviewed Jan 15, 2025

View reviewed changes

src/common/system.rs Outdated Show resolved Hide resolved

GuillaumeGomez reviewed Jan 15, 2025

View reviewed changes

src/common/system.rs Outdated Show resolved Hide resolved

GuillaumeGomez reviewed Jan 15, 2025

View reviewed changes

src/unix/linux/process.rs Outdated Show resolved Hide resolved

Karthik Uthaman added 4 commits January 28, 2025 17:49

address comments

a75f42e

Make sure we add with_thread() in refresh_processes()

bd4cf6d

add test

6ef1e96

documentation

d32056a

sigsegved mentioned this pull request Jan 29, 2025

Refresh thread stats info only if asked #1432

Open

Make test_refresh_tasks deterministic

890bace

GuillaumeGomez reviewed Jan 29, 2025

View reviewed changes

src/common/system.rs Outdated Show resolved Hide resolved

rename thread to tasks

4f8e4e9

Merge branch 'GuillaumeGomez:master' into master

78b9e3d

Karthik Uthaman added 4 commits January 29, 2025 14:02

Impl Default for ProcessRefreshKind

788a0a3

docs

93f9488

Add cfg directive back

306042c

remove imports and use local resolution

8fa146f

GuillaumeGomez reviewed Jan 30, 2025

View reviewed changes

src/common/system.rs Outdated Show resolved Hide resolved

GuillaumeGomez reviewed Jan 30, 2025

View reviewed changes

src/common/system.rs Outdated Show resolved Hide resolved

GuillaumeGomez reviewed Jan 30, 2025

View reviewed changes

src/common/system.rs Outdated Show resolved Hide resolved

GuillaumeGomez reviewed Jan 30, 2025

View reviewed changes

src/common/system.rs Outdated Show resolved Hide resolved

GuillaumeGomez reviewed Jan 30, 2025

View reviewed changes

src/common/system.rs Outdated Show resolved Hide resolved

GuillaumeGomez reviewed Jan 30, 2025

View reviewed changes

src/common/system.rs Outdated Show resolved Hide resolved

GuillaumeGomez reviewed Jan 30, 2025

View reviewed changes

src/common/system.rs Show resolved Hide resolved

GuillaumeGomez reviewed Jan 30, 2025

View reviewed changes

src/unix/linux/process.rs Outdated Show resolved Hide resolved

GuillaumeGomez reviewed Jan 30, 2025

View reviewed changes

tests/process.rs Show resolved Hide resolved

sigsegved and others added 3 commits January 30, 2025 13:22

Update src/common/system.rs

727e438

Co-authored-by: Guillaume Gomez <[email protected]>

Update src/unix/linux/process.rs

432db81

Co-authored-by: Guillaume Gomez <[email protected]>

Address feedback

01aa1e7

GuillaumeGomez reviewed Jan 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce ProcessRefreshKind::Thread #1436

Introduce ProcessRefreshKind::Thread #1436

sigsegved commented Dec 21, 2024

GuillaumeGomez commented Jan 15, 2025

sigsegved commented Jan 29, 2025

GuillaumeGomez commented Jan 29, 2025

sigsegved commented Jan 29, 2025 •

edited

Loading

sigsegved commented Jan 29, 2025

GuillaumeGomez commented Jan 29, 2025

sigsegved commented Jan 29, 2025 •

edited

Loading

GuillaumeGomez commented Jan 29, 2025

sigsegved commented Jan 30, 2025

GuillaumeGomez commented Jan 30, 2025

sigsegved commented Jan 30, 2025

GuillaumeGomez Jan 31, 2025

GuillaumeGomez Jan 31, 2025

GuillaumeGomez Jan 31, 2025

GuillaumeGomez Jan 31, 2025

GuillaumeGomez Jan 31, 2025

GuillaumeGomez Jan 31, 2025

GuillaumeGomez Jan 31, 2025

GuillaumeGomez Jan 31, 2025

GuillaumeGomez commented Jan 31, 2025

-    /// Creates a new `ProcessRefreshKind` with every refresh set to `false`, except for `tasks`
-    /// This is because by default, [`Process`] was fetching all tasks and we want to keep this
-    /// behavior.
+    /// 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.

Introduce ProcessRefreshKind::Thread #1436

Are you sure you want to change the base?

Introduce ProcessRefreshKind::Thread #1436

Conversation

sigsegved commented Dec 21, 2024

GuillaumeGomez commented Jan 15, 2025

sigsegved commented Jan 29, 2025

GuillaumeGomez commented Jan 29, 2025

sigsegved commented Jan 29, 2025 • edited Loading

sigsegved commented Jan 29, 2025

GuillaumeGomez commented Jan 29, 2025

sigsegved commented Jan 29, 2025 • edited Loading

GuillaumeGomez commented Jan 29, 2025

sigsegved commented Jan 30, 2025

GuillaumeGomez commented Jan 30, 2025

sigsegved commented Jan 30, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jan 31, 2025

sigsegved commented Jan 29, 2025 •

edited

Loading

sigsegved commented Jan 29, 2025 •

edited

Loading