List the fd directory in close_open_fds() instead of walking the limit - #456
Conversation
celery#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. celery#417). Errors from os.close() are ignored, matching os.closerange().
There was a problem hiding this comment.
🟡 Changes recommended
A couple of new tests are not fully hermetic/portable (missing os.closerange patch in one fd-dir-path test and an assertion that _open_fds() is non-None even when it can validly be None).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates close_open_fds() to close only the file descriptors that are actually open by listing the platform’s fd directory (/proc/self/fd or /dev/fd) and falling back to the existing os.closerange()-based range logic when fd listing isn’t available, matching CPython’s approach and avoiding O(RLIMIT_NOFILE) loops in constrained/containerized environments.
Changes:
- Add
_FD_DIR,_open_fds(), and FreeBSD/DragonFly fdescfs detection, and use fd-directory enumeration inclose_open_fds()when available. - Extend unit tests to cover both the fd-directory path and the fallback path, plus platform-specific
_open_fds()behavior.
File summaries
| File | Description |
|---|---|
| billiard/compat.py | Implements fd-directory enumeration for open fds with a closerange-based fallback. |
| t/unit/test_compat.py | Adds/extends tests for fd-directory behavior, fallback behavior, and fd-directory detection helpers. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Cygwin is incorrectly routed to /proc/self/fd in both implementation and tests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The change is well-scoped, preserves the existing fallback behavior, and adds targeted tests covering both the new fd-directory path and platform-specific edge cases.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
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.
There was a problem hiding this comment.
🟢 Approval recommended
The change is well-scoped, preserves the existing fallback semantics, and includes targeted unit tests that exercise both the fast-path and fallback-path behavior.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
Follow-up to #455.
close_open_fds()now lists the descriptors that are actually open —/proc/self/fdon Linux,/dev/fdon macOS and on FreeBSD/DragonFly when fdescfs is mounted, following CPython'sFD_DIRinModules/_posixsubprocess.c— and closes only those, skippingkeep. Thecloserange()path from #455 stays as the fallback where no fd directory is available.#455 already made the common case fast: on Linux
os.closerange()isclose_range(2)and finishes in ~0 ms whateverRLIMIT_NOFILEsays. What it doesn't cover is where that syscall isn't available — seccomp profiles that deny it, kernels before 5.9, macOS — where CPython falls back to a C loop over the range. At the container limit from celery/celery#9886 (1,073,741,816) that loop still takes about 3 minutes (measured with a seccomp profile denyingclose_range). Listing the fd directory is what CPython's ownsubprocessdoes in that situation (_close_open_fds_safe), and it doesn't depend on the kernel or the sandbox: measured in the same denied-close_rangecontainer,close_open_fds()with fdmax = 1,073,741,816 → 0 ms.get_fdmax()is untouched. With the fd directory it is never consulted, so there is no need to cap or otherwise adjust its return value (cf. #417).Errors from
os.close()on the fd-directory path are ignored, matchingos.closerange()on the fallback path and CPython's_close_open_fds_safe: closing inherited descriptors in a fresh child is best effort, and re-raising there would surface e.g. EINTR/EIO beforeexecv.Test plan
t/unit/test_compat.py: the fd-directory path (only listed descriptors closed,closerangenever called,keepas ints/file objects/None, close errors ignored,get_fdmaxnever consulted); the fallback path (the Bind the closerange-based close_open_fds() on Python 3 #455 tests, now with_open_fds()forced to None);_open_fds()itself (_FD_DIRfor this platform, a descriptor this process opens is listed, None when the directory is missing, the FreeBSD-without-fdescfs branch, thest_devcheck). Every test that exercises the fd-directory path also patchesos.closerange, so a regression fails the assertion instead of closing the runner's descriptors.main(Bind the closerange-based close_open_fds() on Python 3 #455) all oft/unit/test_compat.pyfails because_open_fds()doesn't exist there yet; the four fallback tests are the Bind the closerange-based close_open_fds() on Python 3 #455 tests with_open_fds()forced to None, so the fallback behaviour itself is unchanged.t/unit: 53 passed, 53 skipped (was 42 + 53). flake8 adds nothing overmainin the two files.