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: process_max_fds is process limit, not OS #314

Merged
merged 3 commits into from Jan 28, 2020
Merged

fix: process_max_fds is process limit, not OS #314

merged 3 commits into from Jan 28, 2020

Conversation

sam-github
Copy link
Contributor

Fix #311

In case anyone is wondering why I don't check for a exact match against the ulimit output, instead of just for "reasonableness", its because there isn't really any way to check the ulimit for the current process other than reading /proc/self/limits. Shelling out to ulimit would gives the correct value, but for the child shell... but that ulimit will be the default for /bin/sh, which might not be the ulimit for the current jest process (and is not on my machine, my login /bin/zsh has a different ulimit than /bin/sh).

@sam-github
Copy link
Contributor Author

Not sure what your CI coverage is, but I manually tested on Linux and Mac (which has no /proc).

@siimon
Copy link
Owner

siimon commented Jan 24, 2020

Thanks, looks good to me! Do you mind update the changelog too?

Thanks!

@sam-github
Copy link
Contributor Author

updated

Copy link
Collaborator

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

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

One performance suggestion, otherwise LGTM!

lib/metrics/processMaxFileDescriptors.js Outdated Show resolved Hide resolved
@sam-github
Copy link
Contributor Author

Is there anything left for me to do?

@zbjornson zbjornson merged commit 9b204e4 into siimon:master Jan 28, 2020
@sam-github sam-github deleted the process-fds-max branch January 28, 2020 20:14
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.

process_max_fds is reporting system limit, not the process limit
3 participants