-
Notifications
You must be signed in to change notification settings - Fork 254
BF: restrict value of get_fdmax #417
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
base: main
Are you sure you want to change the base?
Conversation
In my case I kept finding celery running at 100% and doing nothing. py-spy pointed to the close_open_fds and then ulimit inside the container showed gory detail of ❯ docker run -it --rm --entrypoint bash dandiarchive/dandiarchive-api -c "ulimit -n" 1073741816 situation is not unique to me. See more at dandi/dandi-cli#1488
hm, why pre-commit.ci is even configured if there is no |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets ignore the pre commit. can you elaborate more on the change please? also should we also consider adding some tests to verify the proposed changes?
I would be happy to elaborate! ATM I can only reiterate what tried to describe in original description -- on some systems ulimit would return HUGE number for maximal number of open descriptiors, which would be infeasible to loop through. So, billiard should not try to loop through all the possible billion of them. |
May be we can add some unit tests for the suggested changes as well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
The PR adds logic to cap and warn about excessively high file descriptor limits returned by get_fdmax
, preventing performance issues when iterating open descriptors.
- Capture
os.sysconf('SC_OPEN_MAX')
intofdmax
and handle errors uniformly - Introduce a threshold (100k) to cap
fdmax
to a sensible default (either the passeddefault
or 10 000) and emit a warning - Import
warnings
and emit a deprecation-style warning instead of returning an oversized limit
Comments suppressed due to low confidence (1)
billiard/compat.py:118
- Consider adding unit tests for the new high-value cap branch to verify that large
fdmax
values produce the expected warning and capped return value.
if fdmax >= 1e5:
if fdmax >= 1e5: | ||
# limiting value is ad-hoc and already more than sensible | ||
if default: | ||
fdmax_limited = default | ||
msg = "the default" | ||
else: | ||
fdmax_limited = 10000 |
Copilot
AI
May 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The magic numbers 1e5
and 10000
should be extracted to named constants for clarity and easier future tuning.
if fdmax >= 1e5: | |
# limiting value is ad-hoc and already more than sensible | |
if default: | |
fdmax_limited = default | |
msg = "the default" | |
else: | |
fdmax_limited = 10000 | |
if fdmax >= FD_MAX_LIMIT_THRESHOLD: | |
# limiting value is ad-hoc and already more than sensible | |
if default: | |
fdmax_limited = default | |
msg = "the default" | |
else: | |
fdmax_limited = FD_MAX_DEFAULT_LIMIT |
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
In my case I kept finding celery running at 100% and doing nothing. py-spy pointed to the close_open_fds and then ulimit inside the container showed gory detail of
situation is not unique to me. See more at
I verified that with this fix my celery container gets unstuck and proceeds to report useful errors ;)