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

Fix macos process kinfo #258

Merged
merged 7 commits into from
Jul 27, 2020

Conversation

CosmicHorrorDev
Copy link
Contributor

copy-pasting from rust-psutil/rust-psutil#80 with some small tweaks

In cjbassi/ytop#75 some users, specifically on MacOS, were running into issues where OsError { source: Os { code: 12, kind: Other, message: "Cannot allocate memory" } } was being returned when trying to update the process list.

I eventually tracked this down to processes() that had a couple of issues leading to this problem:

  1. reserve will reserve an additional number of elements represented by the value passed in whereas here it was being given the expected size of all elements in bytes. This was resulting in much more space being reserved than was required.
  2. Incorrectly checking for ENOMEM. Based on this document:

If the amount of data available is greater than the size of the buffer supplied, the call supplies as much data as fits in the buffer provided and returns with the error code ENOMEM.

However

Upon successful completion, the value 0 is returned; otherwise the value -1 is returned and the global variable errno is set to indicate the error.

So I can understand the confusion from the wording, but when it mentions returning ENOMEM this is in the form of errno, not as the return value from the function call. This manifested where the code would check for ENOMEM from sysctl instead of errno, which would cause the error to bubble up instead of appropriately retrying. I think the large reserve size made this case easier to hit since it would stall for more time between reading for the required size and attempting to store.

I would appreciate a thorough look over the changes since it's pretty close to messing with unsafe code and I was able to reproduce the issue on MacOS, but didn't have a chance to verify that the changes work correctly.

@CosmicHorrorDev
Copy link
Contributor Author

It looks like the CI failure is from an unrelated clippy lint

@svartalf
Copy link
Member

Thanks for PR, @LovecraftianHorror! I fixed these lints, can you merge 4269b2a from master into your branch, so we can check the CI status for it?

@CosmicHorrorDev
Copy link
Contributor Author

Pushed and rebased and it looks like some issues did get picked up that I missed since it's gated for just MacOS. I'll get them sorted out and push the updates!

@CosmicHorrorDev
Copy link
Contributor Author

CosmicHorrorDev commented Jul 26, 2020

Hmmm, how would you prefer me to read errno? The other crate I linked used nix, but I saw you were trying to slim down on compile times and that was one of the candidates for removal. It looks like errno defined in sys/unix/bindings.rs uses the same logic as nix but it's not exposed publicly. I could change the access restriction, duplicate the logic, or use nix.

Edit: Moving it into sys/common.rs also seems reasonable and won't expose it in the crate's API

@svartalf
Copy link
Member

Yeah, I definitely want to get rid of the nix crate as a dependency; std::io::Error::raw_os_error() is used mostly across the codebase, while it is not very convinient to match on, this works for now, you can do the same.

@coveralls
Copy link

coveralls commented Jul 26, 2020

Coverage Status

Coverage decreased (-4.4%) to 31.759% when pulling f8275a9 on LovecraftianHorror:fix-macos-process-kinfo into 4269b2a on heim-rs:master.

@CosmicHorrorDev
Copy link
Contributor Author

Thanks for the fast responses. It looks like we're finally all green other than code coverage.

Sorry for the bout of CI driven development. I should have paid more attention to the subtle differences between this crate and the other one I made changes on. I'll have to find the time to get cross set up next time I'm dealing with a platform specific bug.

Copy link
Member

@svartalf svartalf left a comment

Choose a reason for hiding this comment

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

Sorry for the bout of CI driven development.

Don't worry about it, that's why CI exists at all :) master branch is also works as intended now, which is great.

Looks great, thank you once again for pointing the problem! There are two minor things to handle left and I'll merge this PR then

heim-process/src/sys/macos/bindings/process.rs Outdated Show resolved Hide resolved
@CosmicHorrorDev
Copy link
Contributor Author

That should cover the requested change. Thanks for being so helpful in following up with this, it was a joy making this PR!

@svartalf svartalf merged commit 000ada4 into heim-rs:master Jul 27, 2020
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.

None yet

3 participants