Skip to content

BF: restrict value of get_fdmax - #417

Closed
yarikoptic wants to merge 6 commits into
celery:mainfrom
yarikoptic:enh-limit-nofiles
Closed

BF: restrict value of get_fdmax#417
yarikoptic wants to merge 6 commits into
celery:mainfrom
yarikoptic:enh-limit-nofiles

Conversation

@yarikoptic

Copy link
Copy Markdown
Contributor

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

I verified that with this fix my celery container gets unstuck and proceeds to report useful errors ;)

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
@yarikoptic

Copy link
Copy Markdown
Contributor Author

hm, why pre-commit.ci is even configured if there is no .pre-commit-config.yaml?

@auvipy
auvipy self-requested a review March 5, 2025 06:29

@auvipy auvipy left a comment

Copy link
Copy Markdown
Member

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?

@yarikoptic

Copy link
Copy Markdown
Contributor Author

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.

@auvipy

auvipy commented Mar 14, 2025

Copy link
Copy Markdown
Member

May be we can add some unit tests for the suggested changes as well

@yarikoptic

Copy link
Copy Markdown
Contributor Author

sorry -- too busy ATM, feel welcome to contribute some by directly pushing to my branch for this PR, I did

image

@auvipy
auvipy requested a review from Copilot May 21, 2025 05:47

Copilot AI left a comment

Copy link
Copy Markdown

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') into fdmax and handle errors uniformly
  • Introduce a threshold (100k) to cap fdmax to a sensible default (either the passed default 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:

Comment thread billiard/compat.py Outdated
Comment thread billiard/compat.py Outdated
Comment thread billiard/compat.py Outdated
auvipy and others added 4 commits May 21, 2025 05:59
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@GangEunzzang

Copy link
Copy Markdown
Contributor

@yarikoptic @auvipy Thanks for the report and the reproduction — this is the same container setting (ulimit -n = 1073741816) that celery/celery#9886 ran into, and the same function.

Since #455 landed, close_open_fds() no longer walks that range in Python: it calls os.closerange() on the gaps between the kept descriptors, which is close_range(2) on Linux and finishes in ~0 ms at that limit. So the 100% CPU described here should already be gone on main.

What #455 doesn't cover is where close_range(2) isn't available (seccomp denying it, older kernels, macOS) — there os.closerange() falls back to a C loop that still takes about 3 minutes at 1e9. I've sent #456 for that: it lists the fd directory (/proc/self/fd / /dev/fd) and closes only the descriptors actually open, the way CPython's subprocess does, keeping the closerange() path as the fallback. With that, get_fdmax() isn't consulted at all on the fd-directory path, so its return value doesn't need to be capped.

@auvipy auvipy closed this Sep 6, 2026
auvipy added a commit that referenced this pull request Sep 7, 2026
#456)

* List the fd directory in close_open_fds() instead of walking the limit

#455 made close_open_fds() call os.closerange() on the gaps between
kept descriptors, which is close_range(2) on Linux and finishes in
~0 ms whatever RLIMIT_NOFILE says. Where that syscall is unavailable
(seccomp denying it, kernels before 5.9, macOS) os.closerange() falls
back to a C loop over the range, still about 3 minutes at the
container limit from celery/celery#9886.

List /proc/self/fd (or /dev/fd on macOS, and on FreeBSD/DragonFly
when fdescfs is mounted) and close only the descriptors actually
open, the way CPython's subprocess does; keep the closerange() path
as the fallback. get_fdmax() is no longer consulted on the fd
directory path, so its value doesn't need capping (cf. #417). Errors
from os.close() are ignored, matching os.closerange().

* Handle missing fd directory in test

* Patch os.closerange in the remaining fd-directory test

Also wrap the skip message from 85456e4 for flake8 and note gh-148575
in the FD_DIR comment, since Cygwin is only there from that change on.

---------

Co-authored-by: Asif Saif Uddin {"Auvi":"অভি"} <auvipy@gmail.com>
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