Skip to content

Don't hide process creation errors in all_processes#260

Merged
eminence merged 1 commit intoeminence:masterfrom
tatref:process-iter-err
May 10, 2023
Merged

Don't hide process creation errors in all_processes#260
eminence merged 1 commit intoeminence:masterfrom
tatref:process-iter-err

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