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: EMFILE error on macOS Catalina #118

Merged
merged 3 commits into from
Aug 5, 2020
Merged

Conversation

kumakichi
Copy link
Contributor

@kumakichi kumakichi commented Jul 31, 2020

go version go1.14.6 darwin/amd64

versionInfo, err = goversion.ReadExe(path)

goversion.ReadExe will return EMFILE error because of no concurrency goroutines limit in:

func FindAll() []P {

this error will make some Go processes unrecognized, this PR just add a concurrency limit to solve this problem

Copy link
Collaborator

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

FWIW, I cannot reproduce this issue on macOS Catalina 10.15.6 (i.e. I never get EMFILE). How could I reproduce this to verify this PR fixes the issue?

Would also be nice to add a test for it if this is easily reproducible.

@kumakichi
Copy link
Contributor Author

kumakichi commented Aug 4, 2020

Hi, tklauser. I add a test to show the original error i got on my mac(Catalina ,10.15.5)

EMFILE

And I tested on my colleague's computer mac, got the same problem,and his system is also 10.15.5

Copy link
Collaborator

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Thanks for the update and the explanation @kumakichi. Given that the test is dependent on the number of processes on the system the test runs on, it's probably not very reliable and would lead to flaky tests. So I'd rather drop TestEMFILE again. Sorry for the back and forth.

The rest of the change LGTM now, thanks!

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