Skip to content

Use os.closerange instead of counting to 1 billion #425

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

phihag
Copy link

@phihag phihag commented May 14, 2025

In billiard.compat.close_open_fds, after checking whether os.closerange exists, if it exists we did not use it, which makes no sense at all.

Instead, the implementation called os.close, os.sysconf("SC_OPENMAX") - len(keep) times.

On most systems, SC_OPENMAX is something like 1024, 4096, or 65536.
In the latter case, calling it will already take 30ms on my system, almost noticable for humans.

But docker (and some systems) uses a really high value:

docker run --rm python python -c 'import os;print(os.sysconf("SC_OPEN_MAX"))'
1073741816

In other words, when running in docker we were counting to 1 billlion, at 2 million per second.
The single function call close_open_fd([0,1,2]) takes almost 10 minutes there!

The fix is trivial: When os.closerange is available, use the existing implementation. It's right there!

An alternative approach would be to just list all open handles with psutil.Process().open_files() or so, and then just close those few.

In `billiard.compat.close_open_fds`, after checking whether os.closerange exists, if it exists we did _not_ use it, which makes no sense at all.

Instead, the implementation called `os.close`, `os.sysconf("SC_OPENMAX") - len(keep)` times.

On most systems, SC_OPENMAX is something like 1024, 4096, or 65536.
In the latter case, calling it will already take 30ms on my system, almost noticable for humans.

But docker (and some systems) uses a really high value:
```
docker run --rm python python -c 'import os;print(os.sysconf("SC_OPEN_MAX"))'
1073741816
```
In other words, when running in docker we were counting to 1 billlion, at 2 million per second.
The single function call `close_open_fd([0,1,2])` takes almost 10 minutes there!

The fix is trivial: When `os.closerange` is available, use the existing implementation. It's right there!

An alternative approach would be to just list all open handles with `psutil.Process().open_files()` or so, and then just close those few.
@Nusnus Nusnus self-requested a review May 14, 2025 22:43
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

can you please add unit tests to verify this? and there is no regression?

@auvipy auvipy requested a review from Copilot June 2, 2025 11:41
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR simplifies close_open_fds by removing the manual per-FD fallback loop and always using the existing closerange helper, improving performance especially on systems with very high SC_OPEN_MAX.

  • Always invoke closerange to close descriptor ranges instead of iterating each FD
  • Removed the else block that manually closed each FD in reverse
  • No functional changes to get_errno
Comments suppressed due to low confidence (4)

billiard/compat.py:130

  • The custom closerange implementation still iterates over each file descriptor. To leverage the optimized system call when available, consider aliasing closerange = os.closerange if os.closerange exists or calling os.closerange directly.
def closerange(fd_low, fd_high):  # noqa

billiard/compat.py:141

  • [nitpick] The variable names kL and kH are not descriptive. Consider renaming them to something like lower_bounds and upper_bounds (or starts and ends) to clarify their purpose.
for low, high in zip_longest(kL, kH):

billiard/compat.py:134

  • [nitpick] Add a docstring for close_open_fds to describe its behavior, parameters (keep list), and side effects, improving maintainability.
def close_open_fds(keep=None):

billiard/compat.py:134

  • Include unit tests to verify that close_open_fds correctly closes ranges via closerange for very large FD limits and respects the keep list.
def close_open_fds(keep=None):

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.

2 participants