-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
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
subprocess/popen close_fds perform poor if SC_OPEN_MAX is hi #44595
Comments
If the value of sysconf("SC_OPEN_MAX") is high resp. the similar code in popen2.py: There has been an optimization already (range has been replaced by xrange to reduce memory impact), but I think the problem is that for high values of MAXFD, usually a high percentage of the os.close statements will fail, raising an exception (which is an "expensive" operation). I'd like emphasize that this is not a theoretical, but a real world problem: See also: |
Wouldn't it be simpler for you to just don't pass close_fds=True to popen? |
No, I have to use close_fds=True, because I don't want to have the subprocess to inherit each and every file descriptor. |
I understand you don't want the subprocess to inherit "incorrect" file descriptors. However, there are other ways to prevent that from happening:
I understand that there are cases where neither these strategy is not practical, but if you follow it, the performance will be much better, as the closing of unused file descriptor is done in the exec(2) implementation of the operating system. |
Of course I am already closing any files as soon as possible. I know that I could use FD_CLOEXEC. But this would require that I do it explicitly for each descriptor that I use in my program. But this would be a tedious work and require platform-specific coding all around the program. And the whole bunch of python library functions (i.e. the logging module) do not use FD_CLOEXEC as well. |
I've been bitten by this too: close_fds turns my a trivial little monitoring daemon (that reads the output of another status-reporting command once a second or so) into a monster that hogs the entire server. As H. von Bargen says, this kind of degradation should at least come with a warning (or, better, be fixed by C/Pyrex-ifying close_fds). |
This problem has also afflicted us. Attached is a patch which adds closerange(fd_low, fd_high) to the posix I don't really think that this is useful enough to add to the public [python-trunk]$ ./python -m timeit -s 'import python_close' [python-trunk]$ cat python_close.py |
I see that some spaces crept in to the patch. I can fix that (and perhaps |
Finally, I did not include any platform-specific optimizations, as I don't |
Looks good to me. |
Updated patch, now with documentation and test. Patch is against |
I'll review this when I have time. Thanks! If you want speedier |
Reviewed and committed patch as r60097. Thanks for your work! |
Am I wrong or we should reopen this bug. Try |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: