Bind the closerange-based close_open_fds() on Python 3 - #455
Merged
Conversation
The try/except/else around `closerange = os.closerange` put the body that uses closerange() in the except branch and the pure-Python reversed(range(get_fdmax())) loop in the else branch, so the loop is what has run on every Python 3 since 0386dc4. With a container-sized RLIMIT_NOFILE (~1e9) that is about 11 minutes per call (celery/celery#9886). os.closerange exists on every supported Python, so drop the try and keep the closerange-based body. Remove the now unused errno import. Fixes celery#454
There was a problem hiding this comment.
🟢 Approval recommended
The fix directly addresses the incorrect binding logic, keeps behavior well-scoped, and is backed by targeted unit tests for the updated implementation.
Pull request overview
This PR fixes billiard.compat.close_open_fds() so it reliably uses the os.closerange()-based implementation on Python 3 (instead of accidentally binding the pure-Python per-fd loop), addressing the performance regression described in #454—especially when RLIMIT_NOFILE is extremely large.
Changes:
- Replaces the
try/except/elsebinding logic with a singleclose_open_fds()implementation that callsos.closerange()for each gap between kept descriptors. - Removes the now-unused
errnoimport and the unused pure-Python fallback implementation. - Adds unit tests covering
keephandling (ints, file-like objects,None), full-range closing whenkeepis omitted, and ensuring the implementation usescloserange()calls per gap.
File summaries
| File | Description |
|---|---|
billiard/compat.py |
Ensures close_open_fds() always uses os.closerange() to close fd ranges efficiently. |
t/unit/test_compat.py |
Adds focused tests validating range closure behavior and the intended closerange()-based call pattern. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
auvipy
approved these changes
Sep 6, 2026
Member
|
@GangEunzzang thanks for your efforts so far! could you also look intro this pr #417 and possibly take this to fix if needed? I am going to merge the celery pr in the mean time |
This was referenced 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
close_open_fds()is wrapped intry: closerange = os.closerange / except AttributeError: ... / else: .... On Python 3 thetryalways succeeds, so theelsebody is what gets bound — and that is the pure-Pythonfor fd in reversed(range(get_fdmax())): os.close(fd)loop. The body that actually usescloserange()sits in theexceptbranch and has never run. Fixes #454.In containers where
RLIMIT_NOFILEis ~1e9 that is about 11 minutes per call (1,000,000 iterations ≈ 0.6 s measured). It is one of the two loops behind celery/celery#9886; celery/celery#10080 works around it on the celery side, this fixes it here.os.closerangehas existed since Python 2.6 and billiard requires 3.7+, so thetryis dead code: keep thecloserange-based body only and drop the fallback along with the now unusederrnoimport.Test plan
t/unit/test_compat.py(new): onecloserange()call per gap between kept descriptors; file objects andNoneinkeep; nokeepat all; andtest_cost_does_not_grow_with_fdmax, which fails onmain— the old body callsos.close()once per descriptor and never callscloserange().t/unit: 42 passed, 53 skipped (was 38 passed, 53 skipped).