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

Use the proc filesystem to gather process information #1886

Merged
merged 2 commits into from
Oct 20, 2016

Conversation

nmandery
Copy link
Contributor

Description

This pull request uses the proc filesystem to gather process information instead of calling the ps command.

Motivation and Context

See the discussion at PR #1876

@mention-bot
Copy link

@nmandery, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nugend, @Tarrasch and @erikbern to be potential reviewers.

@erikbern
Copy link
Contributor

cool

with open('/proc/{0}/cmdline'.format(pid), 'r') as fh:
return fh.read().replace('\0', ' ').rstrip()
except IOError:
pass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is swallowing this exception about? When and for who will it occur? Maybe you can add a comment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added commit 1836dd2 with a comment

@Tarrasch
Copy link
Contributor

Isn't it better to actually throw an exception instead of using a fallback-method when there's a file permissions error? I imagine an admin mis-configuring so that two different users run the same command. It's better to report an error early rather than always running in a --no-lock fashion. Have I misunderstood anything?

@nmandery
Copy link
Contributor Author

The former implementation as well as the implementation for windows systems both do not raise any errors when a pid can not be resolved. Reasons I can think of may be missing permissions or simply that the process in the meantime has exited. Instead the fallback at the end of the function is used. So there is always a string retruned giving some description on the process.

This PR simply preserves this behavior of the getpcmp function. The function tries to resolve the command of the given pid and returns the most meaningful description possible.

In case the desired behavior of getpcmd has changed to raise exceptions when resolving the command fails, I do not see it as sufficient to propagate the IOError from the failing access to /proc. The windows implementation would also be in need of a patch to also raise an exception when the pid can not be found in the output of the wmic command. This patch would then result in making the fallback at the end of the function non-reachable and obsolete.

Maybe you should also discuss this with @erikbern , the original author of the getpcmd function

@Tarrasch
Copy link
Contributor

@nmandery, You seem to have thought about this more thoroughly than I have. And admittedly know more about OS/procs/etc than me. :) Furthermore you just documented your thought-process in case further committers want to read up on this. Thanks. I feel confident enough to merge this. :)

@Tarrasch Tarrasch merged commit 632f14f into spotify:master Oct 20, 2016
@Tarrasch
Copy link
Contributor

@nmandery, I took the liberty to myself summarize why we did this in the squash-commit I just merged this PR with. I hope it looks good.

@nmandery
Copy link
Contributor Author

@Tarrasch Looks fine to me. Thank you for merging and the reviews.

This was referenced Jun 29, 2022
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.

4 participants