Skip to content
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

Closed
hvbargen mannequin opened this issue Feb 19, 2007 · 14 comments
Closed

subprocess/popen close_fds perform poor if SC_OPEN_MAX is hi #44595

hvbargen mannequin opened this issue Feb 19, 2007 · 14 comments
Assignees
Labels
performance Performance or resource usage stdlib Python modules in the Lib dir

Comments

@hvbargen
Copy link
Mannequin

hvbargen mannequin commented Feb 19, 2007

BPO 1663329
Nosy @gvanrossum, @loewis, @birkenfeld, @ssbarnea
Files
  • posix_closerange.patch
  • posix_closerange.patch: Updated patch
  • 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:

    assignee = 'https://github.com/birkenfeld'
    closed_at = <Date 2008-01-19.20:22:46.047>
    created_at = <Date 2007-02-19.10:17:38.000>
    labels = ['library', 'performance']
    title = 'subprocess/popen close_fds perform poor if SC_OPEN_MAX is hi'
    updated_at = <Date 2020-04-24.00:29:24.147>
    user = 'https://bugs.python.org/hvbargen'

    bugs.python.org fields:

    activity = <Date 2020-04-24.00:29:24.147>
    actor = 'vstinner'
    assignee = 'georg.brandl'
    closed = True
    closed_date = <Date 2008-01-19.20:22:46.047>
    closer = 'georg.brandl'
    components = ['Library (Lib)']
    creation = <Date 2007-02-19.10:17:38.000>
    creator = 'hvbargen'
    dependencies = []
    files = ['8552', '9070']
    hgrepos = []
    issue_num = 1663329
    keywords = ['patch']
    message_count = 14.0
    messages = ['31289', '31290', '31291', '31292', '31293', '31294', '56490', '56491', '56492', '57016', '59281', '59289', '60224', '160956']
    nosy_count = 7.0
    nosy_names = ['gvanrossum', 'loewis', 'georg.brandl', 'pjdelport', 'klaas', 'hvbargen', 'ssbarnea']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = 'performance'
    url = 'https://bugs.python.org/issue1663329'
    versions = ['Python 2.6']

    @hvbargen
    Copy link
    Mannequin Author

    hvbargen mannequin commented Feb 19, 2007

    If the value of sysconf("SC_OPEN_MAX") is high
    and you try to start a subprocess with subprocess.py or os.popen2 with close_fds=True, then starting the other process is very slow.
    This boils down to the following code in subprocess.py:
    def _close_fds(self, but):
    for i in xrange(3, MAXFD):
    if i == but:
    continue
    try:
    os.close(i)
    except:
    pass

    resp. the similar code in popen2.py:
    def _run_child(self, cmd):
    if isinstance(cmd, basestring):
    cmd = ['/bin/sh', '-c', cmd]
    for i in xrange(3, MAXFD):
    try:
    os.close(i)
    except OSError:
    pass

    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).
    It has been suggested already to add a C implementation called "rclose" or "close_range" that tries to close all FDs in a given range (min, max) without the overhead of Python exception handling.

    I'd like emphasize that this is not a theoretical, but a real world problem:
    We have a Python application in a production environment on Sun Solaris. Some other software running on the same server needed a high value of 260000 for SC_OPEN_MAX (set with ulimit -n XXX or in some /etc/-file (don't know which one).
    Suddenly calling any other process with subprocess.Popen (..., close_fds=True) now took 14 seconds (!) instead of some microseconds.
    This caused a huge performance degradation, since the subprocess itself only needs only a few seconds.

    See also:
    Patches item bpo-1607087 "popen() slow on AIX due to large FOPEN_MAX value".
    This contains a fix, but only for AIX - and I think the patch does not support the "but" argument used in subprocess.py.
    The correct solution should be coded in C, and should
    do the same as the _close_fds routine in subprocess.py.
    It could be optimized to make use of (operating-specific) system calls to close all handles from (but+1) to MAX_FD with "closefrom" or "fcntl" as proposed in the patch.

    @hvbargen hvbargen mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Feb 19, 2007
    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Feb 20, 2007

    Wouldn't it be simpler for you to just don't pass close_fds=True to popen?

    @hvbargen
    Copy link
    Mannequin Author

    hvbargen mannequin commented Feb 21, 2007

    No, I have to use close_fds=True, because I don't want to have the subprocess to inherit each and every file descriptor.
    This is for two reasons:
    i) Security - why should the subproces be able to work with all the parent processes' files?
    ii) Sometimes, for whatever reason, the subprocess (Oracle Reports in this case) seems to hang. And because it inherited all of the parent's log file handles, the paraent can not close and remove its log files correctly. This is the reason why I stumbled about close_fds at all. BTW on MS Windows, a similar (but not equivalent) solution was to create the log files as non-inheritable.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Feb 21, 2007

    I understand you don't want the subprocess to inherit "incorrect" file descriptors. However, there are other ways to prevent that from happening:

    • you should close file descriptors as soon as you are done with the files
    • you should set the FD_CLOEXEC flag on all file descriptors you don't want to be inherited, using fnctl(fd, F_SETFD, 1)

    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.

    @hvbargen
    Copy link
    Mannequin Author

    hvbargen mannequin commented Feb 22, 2007

    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.
    Right now, more or less the only platform specific code in the program is where I call subprocesses, and I like to keep it that way.
    The same is true for the socket module. All sockets are by default inherited to child processes.
    So, the only way to prevent unwanted handles from inheriting to child processes, is in fact to specify close_fds=True in subprocess.py.
    If you think that a performance patch similar to the patch #16078087 makes no sense, then the close_fds argument should either be marked as deprecated or at least the documentation should mention that the implementation is slow for large values of SC_OPEN_MAX.

    @pjdelport
    Copy link
    Mannequin

    pjdelport mannequin commented Mar 18, 2007

    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).

    @klaas
    Copy link
    Mannequin

    klaas mannequin commented Oct 16, 2007

    This problem has also afflicted us.

    Attached is a patch which adds closerange(fd_low, fd_high) to the posix
    (and consequently os) module, and modifies subprocess to use it. Patch
    is against trunk but should work for 2.5maint.

    I don't really think that this is useful enough to add to the public
    api, but it results in a huge performance benefit for subprocess:

    [python-trunk]$ ./python -m timeit -s 'import python_close'
    'python_close.go(100000)'
    10 loops, best of 3: 358 msec per loop
    [python-trunk]$ ./python -m timeit -s 'import os' 'os.closerange(4,
    100000)'
    10 loops, best of 3: 20.7 msec per loop
    [python-trunk]$ ./python -m timeit -s 'import python_close'
    'python_close.go(300000)'
    10 loops, best of 3: 1.05 sec per loop
    [python-trunk]$ ./python -m timeit -s 'import os' 'os.closerange(4,
    300000)'10 loops, best of 3: 63 msec per loop

    [python-trunk]$ cat python_close.py
    import os, sys
    def go(N):
    for i in xrange(4, N):
    try:
    os.close(i)
    except:
    pass

    @klaas
    Copy link
    Mannequin

    klaas mannequin commented Oct 16, 2007

    I see that some spaces crept in to the patch. I can fix that (and perhaps
    rename the function to start with an underscore) if desired.

    @klaas
    Copy link
    Mannequin

    klaas mannequin commented Oct 16, 2007

    Finally, I did not include any platform-specific optimizations, as I don't
    have AIX or solaris boxes on which to test them (and they can easily be
    added later). An order-mag speedup on all *nix platforms is a good start.

    @gvanrossum
    Copy link
    Member

    Looks good to me.

    @klaas
    Copy link
    Mannequin

    klaas mannequin commented Jan 5, 2008

    Updated patch, now with documentation and test. Patch is against
    2.6trunk, but probably will apply against 3.0 as well

    @klaas klaas mannequin added stdlib Python modules in the Lib dir and removed interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Jan 5, 2008
    @gvanrossum
    Copy link
    Member

    I'll review this when I have time. Thanks! If you want speedier
    response, please find someone else among the core developers; or perhaps
    the bug day on Jan 19 will help.

    @birkenfeld
    Copy link
    Member

    Reviewed and committed patch as r60097. Thanks for your work!

    @ssbarnea
    Copy link
    Mannequin

    ssbarnea mannequin commented May 17, 2012

    Am I wrong or we should reopen this bug. Try ulimit -n unlimited and you'll make python code that does popen practically blocking.

    @ssbarnea ssbarnea mannequin added performance Performance or resource usage labels May 17, 2012
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    performance Performance or resource usage stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants