Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't hide process creation errors in all_processes #260

Merged
merged 1 commit into from
May 10, 2023
Merged

Don't hide process creation errors in all_processes #260

merged 1 commit into from
May 10, 2023

Conversation

tatref
Copy link
Contributor

@tatref tatref commented Mar 27, 2023

This is followup of #188

Current implementation hide Process creation errors in all_processes, making this function nearly useless in a resource constrained environment. Errors are silently ignored, making look like only a few processes exist.

The fact that FDs are created for each process is not necessarily an issue, but users of the library must have a way to know if any operation worked as expected or not.

This PR shouldn't change much for users, because the Processes are wrapped in a Result either way

Example usage

let all_processes: Vec<Process> = procfs::process::all_processes()
    .unwrap()
    .filter_map(|p| match p {
        Ok(p) => Some(p),                              // happy path
        Err(e) => match e {
            procfs::ProcError::NotFound(_) => None,    // process vanished during iteration
            procfs::ProcError::Io(e, path) => None,    // can match on path to decide if we can continue
            x => {
                panic!("Can't read process {x:?}");    // some unknown error
            }
        },
    })
    .collect();

@tatref tatref marked this pull request as ready for review March 27, 2023 23:00
@tatref
Copy link
Contributor Author

tatref commented May 3, 2023

I rebased from master

@eminence
Copy link
Owner

Thanks, this change makes sense. As a followup TODO, some additional documentation about strategies to handle errors might be good (similar to the comment you made in the PR)

@eminence eminence merged commit 894bd23 into eminence:master May 10, 2023
@tatref tatref deleted the process-iter-err branch May 10, 2023 20:31
@tatref tatref mentioned this pull request May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants