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

File descriptor leaks in os.scandir() #70182

Closed
serhiy-storchaka opened this issue Jan 2, 2016 · 27 comments
Closed

File descriptor leaks in os.scandir() #70182

serhiy-storchaka opened this issue Jan 2, 2016 · 27 comments
Assignees
Labels
extension-modules C modules in the Modules dir performance Performance or resource usage

Comments

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Jan 2, 2016

BPO 25994
Nosy @gvanrossum, @vstinner, @benhoyt, @vadmium, @serhiy-storchaka, @MojoVampire
Files
  • scandir_close.patch
  • scandir_close_2.patch
  • scandir_close_3.patch
  • scandir_close_4.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/serhiy-storchaka'
    closed_at = <Date 2016-02-11.11:41:09.240>
    created_at = <Date 2016-01-02.17:48:16.212>
    labels = ['extension-modules', 'performance']
    title = 'File descriptor leaks in os.scandir()'
    updated_at = <Date 2016-02-11.11:53:27.729>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2016-02-11.11:53:27.729>
    actor = 'vstinner'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2016-02-11.11:41:09.240>
    closer = 'serhiy.storchaka'
    components = ['Extension Modules']
    creation = <Date 2016-01-02.17:48:16.212>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = ['41857', '41869', '41873', '41881']
    hgrepos = []
    issue_num = 25994
    keywords = ['patch']
    message_count = 27.0
    messages = ['257351', '257377', '257391', '257398', '258017', '258040', '258045', '258047', '258233', '258239', '258242', '258244', '259867', '259891', '259901', '259904', '259920', '259937', '259939', '259947', '259968', '259996', '259998', '260041', '260087', '260094', '260096']
    nosy_count = 7.0
    nosy_names = ['gvanrossum', 'vstinner', 'benhoyt', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'josh.r']
    pr_nums = []
    priority = 'high'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'resource usage'
    url = 'https://bugs.python.org/issue25994'
    versions = ['Python 3.6']

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Jan 2, 2016

    os.scandir() opens a file descriptor and closes it only in its destructor. This can causes file descriptor leaks in Python implementations without reference counting and if the scandir iterator becomes a part of reference loop or long living object. Since the number of open file descriptors is limited, this can leads to problems.

    We need to add the close() method to the scandir iterator (as in files and generators). It would be useful also to make it a context manager.

    In 3.5 we have to add a warning about this behavior.

    @serhiy-storchaka serhiy-storchaka added extension-modules C modules in the Modules dir performance Performance or resource usage labels Jan 2, 2016
    @vstinner
    Copy link
    Member

    vstinner commented Jan 2, 2016

    Hi,

    If I recall correctly, this issue was discussed in the long review of os.scandir(): issue bpo-22524.

    "os.scandir() opens a file descriptor and closes it only in its destructor."

    Hopefully, it's incorrect: it's closed when the iterator is exhausted. See how ScandirIterator_close() is used.

    "This can causes file descriptor leaks in Python implementations without reference counting"

    Destructors are part of the Python language. Are you aware of a Python implementation *never* calls destructors? I know that PyPy can call destructors "later", not necessary when the last reference to the object is removed. Do you think that we may reach the file descriptor limit because of that?

    "We need to add the close() method to the scandir iterator (as in files and generators)."

    Is it part of the iterator protocol? Or do you mean to explicitly call close()?

    "It would be useful also to make it a context manager."

    If we decide to add a close() method, it like the idea of also supporting the context manager protocol.

    "In 3.5 we have to add a warning about this behavior."

    Yeah, maybe we can elaborate the doc to explain how the file descriptor / Windows handle is used.

    @benhoyt
    Copy link
    Mannequin

    benhoyt mannequin commented Jan 3, 2016

    I'm not sure this is actually a leak, because (looking at the code) ScandirIterator_close() is called not just in the destructor, but also at the end of iterating, just before raising StopIteration. Is the issue that if an exception is raised or you stop iterating before the end, then it's less determined when the destructor/close is called?

    I think we could fairly easily add an explicit close method to the iterator and make it a context manager (add __enter__ and __exit__ methods to the iterator). Is this what you're thinking of:

        # manual close
        it = scandir.scandir(path)
        first_entry = next(it)
        it.close()
    
        with scandir.scandir(path) as it:
            first_entry = next(it)

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Jan 3, 2016

    Yes, it's what I mean. Add methods close, __enter__ and __exit__ to the iterator. The scandir iterator is not just iterator, it is like file object.

    And as in file object, we perhaps have to emit a ResourceWarning in the destructor if close() or __exit__() were not called.

    @gvanrossum
    Copy link
    Member

    gvanrossum commented Jan 11, 2016

    Note there's also a nasty corner case related to generators and GC. If a generator contains a with-block or finally-clause, and the generator is not run until its end because the caller hit an exception on one of the items returned, and the generator object is somehow kept alive (either because it's stored in some longer-living state or because the Python implementation doesn't use reference counting) then the close() call in the finally-clause or in the __exit__ method doesn't run until the generator object is garbage-collected.

    IOW even making it a context manager doesn't completely solve this issue (though it can certainly help).

    @vadmium
    Copy link
    Member

    vadmium commented Jan 11, 2016

    Guido are you saying in the following code, the “finally” message is not guaranteed to be printed out? Or just that you cannot limit a ResourceWarning to garbage collection?

    def g():
        try:
            yield "item"
        finally:
            # Run at exhaustion, close(), and garbage collection
            print("finally")
    
    gi = g()
    try:
        item = next(gi)
        print(item / 2)  # Oops, TypeError
    finally:
        # Should be run as the exception passes through. GeneratorExit is raised inside the generator, causing the other “finally” block to execute. All before the original exception is caught and a traceback is printed.
        gi.close()

    But as I understand it, os.scandir() is not a generator, so none of these problems would apply.

    @gvanrossum
    Copy link
    Member

    gvanrossum commented Jan 12, 2016

    It was a bit more subtle. I think like this:

    def f():
        with some_lock:
            yield 0
            yield 1
    
    def g():
        with another_lock:
            it = f()
            for i in it:
                raise

    We determined that another_lock was freed *before* some_lock. This is because the local variable 'it' keeps the generator object returned by f() alive until after the with-block in g() has released another_lock.

    I think this does not strictly require f to be a generator -- it's any kind of closable resource that only gets closed by its destructor.

    The solution is to add a try/finally that calls it.close() before leaving the with-block in g().

    Hope this helps.

    @vadmium
    Copy link
    Member

    vadmium commented Jan 12, 2016

    Okay I understand. You have to explicitly call the close() method if you want a generator to be cleaned up properly, which parallels how Serhiy’s proposal would be used.

    @vadmium
    Copy link
    Member

    vadmium commented Jan 14, 2016

    I definitely support adding a close() method, and a ResourceWarning if the iterator is not exhausted when it is deallocated. Adding context manager support is probably worthwhile as well, though there is one disadvantage: it would be hard to implement scandir() as a plain generator, because generators don’t support the context manager protocol. But C Python’s implementation is in C, and even in native Python you could implement it as a custom iterator class.

    I suggest using this issue for adding the new API(s), presumably to 3.6 only, and bpo-26111 for adding a warning to the existing 3.5 documentation.

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Jan 14, 2016

    If scandir() is implemented as native Python generator (see for example bpo-25911), it could easily be converted to context manager:

    def scandir(path):
        return contextlib.closing(native_scandir(path))
    
    def native_scandir(path):
        ...
        yield ...

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Jan 14, 2016

    Since the close() method can be added only in 3.6, but we have leaks in 3.5 too, I suggest to closed file descriptor if error happened during iteration (bpo-26117). This will allow to write safe code even in 3.5.

    @vadmium
    Copy link
    Member

    vadmium commented Jan 14, 2016

    Contextlib.closing() cannot be used in general, because it doesn’t inherit the iterator protocol from the wrapped generator. So I think you really need a class that implements __exit__(), __iter__(), etc at the same time.

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Feb 8, 2016

    Proposed patch adds the close() methon and the support of the context manager protocol for the os.scandir class.

    @vstinner
    Copy link
    Member

    vstinner commented Feb 8, 2016

    Context manager protocol, close() method: it looks more and more like a file. In this case, I suggest to emit a ResourceWarning in the destructor if it's not closed explicitly. It mean that the scandir() must always be used with "with" or at least that close() is always closed.

    We probably need to update the code to be more explicitly on that.

    @MojoVampire
    Copy link
    Mannequin

    MojoVampire mannequin commented Feb 9, 2016

    Adding a ResourceWarning even if the generator is run to completion? That seems... dev hostile. I mean, yes, probably best to document it as best practice to use with with statement, but something simple like files = sorted(os.scandir('.'), key=lambda x: x.stat().st_mtime) to get files ordered by modification time (which cleanly runs the generator to exhaustion immediately) should not be raising ResourceWarning in 3.6 when it didn't do so in 3.5, and has minimal risk of leaking in any event.

    @vadmium
    Copy link
    Member

    vadmium commented Feb 9, 2016

    I would be in favour of adding a ResourceWarning in 3.6 if the iterator is garbage collected without being exhausted. But as Josh says, it might be overkill emitting a warning when we already know the iterator has been finished and cleaned up.

    @vstinner
    Copy link
    Member

    vstinner commented Feb 9, 2016

    Josh Rosenberg added the comment:

    Adding a ResourceWarning even if the generator is run to completion?

    I'm ok to only emit the warning is the generator is not exhausted.

    The warning would be emited in the destructor if the generator is not
    closed. In practice, the generator is *already* closed on the last
    file, so the warning would not be emited if the generator is
    exhausted.

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Feb 9, 2016

    Updated patch adds a resource warning, tests, improves the documentation, and addresses other comments.

    @vstinner
    Copy link
    Member

    vstinner commented Feb 9, 2016

    Updated patch adds a resource warning, tests, improves the documentation, and addresses other comments.

    Review sent.

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Feb 9, 2016

    Updated patch addresses new Victor's comments.

    @vadmium
    Copy link
    Member

    vadmium commented Feb 10, 2016

    About testing that list(iterator) is empty after the iterator is closed, IMO this is an implementation detail. It would be equally (or more) sensible to raise an exception, like proposed for “async def” coroutines in bpo-25887. I suppose the main purpose of those tests is to ensure there is no leakage or warning, so maybe add some more filterwarnings() checks in those tests instead?

    I left some other review comments.

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Feb 10, 2016

    Updated patch addresses Martins comments.

    @vstinner
    Copy link
    Member

    vstinner commented Feb 10, 2016

    scandir_close_4.patch LGTM, I just added a comment on test_os.py, you may factorize the code.

    @vstinner
    Copy link
    Member

    vstinner commented Feb 10, 2016

    I just added a comment on test_os.py, you may factorize the code.

    Ah, Serhiy created the issue bpo-26325 for that. Nice.

    It's up to you to commit this change, or bpo-26325 first, both are good now ;)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 11, 2016

    New changeset f98ed0616d07 by Serhiy Storchaka in branch 'default':
    Issue bpo-25994: Added the close() method and the support of the context manager
    https://hg.python.org/cpython/rev/f98ed0616d07

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Feb 11, 2016

    Committed with using the helper from bpo-26325 for testing.

    Thank you all for your reviews, especially for help with the documentation.

    @vstinner
    Copy link
    Member

    vstinner commented Feb 11, 2016

    Committed with using the helper from bpo-26325 for testing.

    Great! Good job.

    @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
    extension-modules C modules in the Modules dir performance Performance or resource usage
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants