[issue25994] File descriptor leaks in os.scandir()

STINNER Victor added the comment: > Committed with using the helper from issue26325 for testing. Great! Good job. -- ___ Python tracker ___ _

[issue25994] File descriptor leaks in os.scandir()

Serhiy Storchaka added the comment: Committed with using the helper from issue26325 for testing. Thank you all for your reviews, especially for help with the documentation. -- assignee: -> serhiy.storchaka resolution: -> fixed stage: patch review -> resolved status: open -> closed __

[issue25994] File descriptor leaks in os.scandir()

Roundup Robot added the comment: New changeset f98ed0616d07 by Serhiy Storchaka in branch 'default': Issue #25994: Added the close() method and the support of the context manager https://hg.python.org/cpython/rev/f98ed0616d07 -- nosy: +python-dev ___

[issue25994] File descriptor leaks in os.scandir()

STINNER Victor added the comment: > I just added a comment on test_os.py, you may factorize the code. Ah, Serhiy created the issue #26325 for that. Nice. It's up to you to commit this change, or #26325 first, both are good now ;) -- ___ Python track

[issue25994] File descriptor leaks in os.scandir()

STINNER Victor added the comment: scandir_close_4.patch LGTM, I just added a comment on test_os.py, you may factorize the code. -- ___ Python tracker ___ ___

[issue25994] File descriptor leaks in os.scandir()

Serhiy Storchaka added the comment: Updated patch addresses Martins comments. -- Added file: http://bugs.python.org/file41881/scandir_close_4.patch ___ Python tracker ___ ___

[issue25994] File descriptor leaks in os.scandir()

Martin Panter added the comment: 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 Issue 25887. I suppose the main purpose of t

[issue25994] File descriptor leaks in os.scandir()

Serhiy Storchaka added the comment: Updated patch addresses new Victor's comments. -- Added file: http://bugs.python.org/file41873/scandir_close_3.patch ___ Python tracker ___ __

[issue25994] File descriptor leaks in os.scandir()

STINNER Victor added the comment: > Updated patch adds a resource warning, tests, improves the documentation, and > addresses other comments. Review sent. -- ___ Python tracker ___

[issue25994] File descriptor leaks in os.scandir()

Serhiy Storchaka added the comment: Updated patch adds a resource warning, tests, improves the documentation, and addresses other comments. -- Added file: http://bugs.python.org/file41869/scandir_close_2.patch ___ Python tracker

[issue25994] File descriptor leaks in os.scandir()

STINNER Victor added the comment: 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

[issue25994] File descriptor leaks in os.scandir()

Martin Panter added the comment: 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. --

[issue25994] File descriptor leaks in os.scandir()

Josh Rosenberg added the comment: 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.st

[issue25994] File descriptor leaks in os.scandir()

STINNER Victor added the comment: 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()

[issue25994] File descriptor leaks in os.scandir()

Serhiy Storchaka added the comment: Proposed patch adds the close() methon and the support of the context manager protocol for the os.scandir class. -- keywords: +patch stage: -> patch review versions: -Python 3.5 Added file: http://bugs.python.org/file41857/scandir_close.patch _

[issue25994] File descriptor leaks in os.scandir()

Martin Panter added the comment: 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. -- ___

[issue25994] File descriptor leaks in os.scandir()

Serhiy Storchaka added the comment: 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 (issue26117). This will allow to write safe code even in 3.5. --

[issue25994] File descriptor leaks in os.scandir()

Serhiy Storchaka added the comment: If scandir() is implemented as native Python generator (see for example issue25911), it could easily be converted to context manager: def scandir(path): return contextlib.closing(native_scandir(path)) def native_scandir(path): ... yield ...

[issue25994] File descriptor leaks in os.scandir()

Martin Panter added the comment: 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()

[issue25994] File descriptor leaks in os.scandir()

Martin Panter added the comment: 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. -- ___ Python tracker

[issue25994] File descriptor leaks in os.scandir()

Guido van Rossum added the comment: 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. T

[issue25994] File descriptor leaks in os.scandir()

Martin Panter added the comment: 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(

[issue25994] File descriptor leaks in os.scandir()

Guido van Rossum added the comment: 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

[issue25994] File descriptor leaks in os.scandir()

Serhiy Storchaka added the comment: 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__() w

[issue25994] File descriptor leaks in os.scandir()

Ben Hoyt added the comment: 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 b

[issue25994] File descriptor leaks in os.scandir()

STINNER Victor added the comment: Hi, If I recall correctly, this issue was discussed in the long review of os.scandir(): issue #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

[issue25994] File descriptor leaks in os.scandir()

New submission from Serhiy Storchaka: 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