[issue11767] Maildir iterator leaks file descriptors by default

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

Python 2.7 is no longer supported.

--
nosy: +serhiy.storchaka
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11767] Maildir iterator leaks file descriptors by default

[Ezio Melotti]

Changes by Ezio Melotti ezio.melo...@gmail.com:


--
stage:  - patch review

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11767
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11767] Maildir iterator leaks file descriptors by default

[Roundup Robot]

Roundup Robot devnull@devnull added the comment:

New changeset fea1920ae75f by R David Murray in branch '3.2':
#11767: use context manager to close file in __getitem__ to prevent FD leak
http://hg.python.org/cpython/rev/fea1920ae75f

New changeset 1d7a91358517 by R David Murray in branch 'default':
merge #11767: use context manager to close file in __getitem__ to prevent FD 
leak
http://hg.python.org/cpython/rev/1d7a91358517

--
nosy: +python-dev

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11767
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11767] Maildir iterator leaks file descriptors by default

[R. David Murray]

R. David Murray rdmur...@bitdance.com added the comment:

The patch doesn't work on 2.7.  The failures are related to #11700, although 
that would seem to indicate that something *is* trying to close the file.  I 
have no idea what.

--
dependencies: +mailbox.py proxy updates

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11767
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11767] Maildir iterator leaks file descriptors by default

[Filip Gruszczyński]

Filip Gruszczyński grusz...@gmail.com added the comment:

Should I try to do something about this right now or should I wait until #1170 
is finishedclosed and only then try to fix this issue too?

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11767
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11767] Maildir iterator leaks file descriptors by default

[R. David Murray]

R. David Murray rdmur...@bitdance.com added the comment:

Well, if you want to you could investigate further, and try the patch from 
#11700 and see what is left to do here after it has been applied.  I'll try to 
get 11700 in soon, though.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11767
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11767] Maildir iterator leaks file descriptors by default

[R. David Murray]

R. David Murray rdmur...@bitdance.com added the comment:

Well, it turns out I was totally wrong about the 11700 dependency.  I misread 
the errors that were produced by the test suite.  Even after fixing 11700 they 
are still there: the tests are reading from the closed files.  So something 
changed in the logic between 2.7 and 3.x.  It is possible that this is related 
to the addition of context manager support, and is almost certainly related to 
the general cleanup of unclosed files that was done after Antoine's 
ResourceWarning patch.

Filip, do you want to investigate this?  We always have the option of ignoring 
the problem in 2.7, but since you reported it against 2.7 I'm guessing you'd 
like to see it fixed there.

I'm attaching a version of the patch that applies cleanly to 2.7 tip.

--
dependencies:  -mailbox.py proxy close method cannot be called multiple times
Added file: http://bugs.python.org/file22400/mailbox_fd_loss.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11767
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11767] Maildir iterator leaks file descriptors by default

[Terry J. Reedy]

Changes by Terry J. Reedy tjre...@udel.edu:


--
versions:  -Python 2.5, Python 2.6

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11767
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11767] Maildir iterator leaks file descriptors by default

[Filip Gruszczyński]

Filip Gruszczyński grusz...@gmail.com added the comment:

Here is a new patch, that uses with in __getitem__. I wonder, if we shouldn't 
check for an AttributeError in case object returned by get_file doesn't have 
__exit__ method, but does have close and use close instead of with. But it's 
for you to decide, as I am very fresh to with statement.

--
Added file: http://bugs.python.org/file21569/11767_2.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11767
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11767] Maildir iterator leaks file descriptors by default

[R. David Murray]

R. David Murray rdmur...@bitdance.com added the comment:

Ah, that's exactly why I suggested using the 'closing' context manager from 
contextlib.  That context manager returns the object passed to it, and then 
when its __exit__ method is called, calls the close method of that object that 
was passed to it.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11767
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11767] Maildir iterator leaks file descriptors by default

[Filip Gruszczyński]

Filip Gruszczyński grusz...@gmail.com added the comment:

I am sorry. This is the first time I see contextlib and haven't understood, 
that I should use a function from it. Here is a version with mock object having 
close method and __getitem__ using contextlib.closing.

--
Added file: http://bugs.python.org/file21572/11767_3.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11767
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11767] Maildir iterator leaks file descriptors by default

[R. David Murray]

R. David Murray rdmur...@bitdance.com added the comment:

I shouldn't have assumed that you knew about contextlib, and should have 
mentioned it by name the first time.

The patch looks good to me.  Not sure it is necessary to loop through ten fake 
mailboxes, but it doesn't hurt, either.  (Other potential reviewers note: 
'looks' is the operative word, I haven't tested it myself yet.)

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11767
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11767] Maildir iterator leaks file descriptors by default

[Filip Gruszczyński]

Filip Gruszczyński grusz...@gmail.com added the comment:

Shouldn't this be your responsibility to close this descriptor in the object 
used as self._factory? When self._factory is called it should read from the 
file like object and then simply close it, just as get_message (when 
self._factory is not provided) does.

--
nosy: +gruszczy

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11767
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11767] Maildir iterator leaks file descriptors by default

[Brendan Dolan-Gavitt]

Brendan Dolan-Gavitt brenda...@gatech.edu added the comment:

The _factory object didn't open the file, so why should it close it?

It's also worth noting that things one would naturally consider using as 
factories (like email.message_from_file) don't close the fd when they're done. 
I will grant that you could easily create a wrapper factory that does close the 
descriptor in this case, but that's not the obvious thing to do.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11767
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11767] Maildir iterator leaks file descriptors by default

[R. David Murray]

R. David Murray rdmur...@bitdance.com added the comment:

It is not clear from the docs what the expected behavior of factory is.  It is 
certainly the case that the custom classes used by mailbox by default do not 
close the file they are passed.  So either those classes should close the input 
file and factory should be so documented, or all the methods that create a 
message object should close the input file when they create one.  Since the 
existing get_message methods take care to close any opened input files, I favor 
the latter (ie: the OP's suggestion).

Anyone care to propose a patch?

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11767
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11767] Maildir iterator leaks file descriptors by default

[R. David Murray]

Changes by R. David Murray rdmur...@bitdance.com:


Removed file: http://bugs.python.org/file21554/11767.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11767
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11767] Maildir iterator leaks file descriptors by default

[R. David Murray]

R. David Murray rdmur...@bitdance.com added the comment:

get_file's promise is that what is returned is a file like object, so it not 
having a close() method would be an error.  So I don't think you need the 
try/except.  What I would suggest is to use the 'closing' context manager 
around the return statement.

As for tests, you could create a subclass with a custom get_file method that 
returns a mock object you can test to make sure the close method gets called, 
since Mailbox is the only place __getitem__ is defined.

Gah, I hit the wrong key an deleted your patch.  Reattaching.

--
Added file: http://bugs.python.org/file21557/11767.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11767
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11767] Maildir iterator leaks file descriptors by default

[Antoine Pitrou]

Changes by Antoine Pitrou pit...@free.fr:


--
nosy: +r.david.murray

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11767
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11767] Maildir iterator leaks file descriptors by default

[Brendan Dolan-Gavitt]

New submission from Brendan Dolan-Gavitt brenda...@gatech.edu:

The default constructor for Maildir is rfc822.Message. This means that when 
iterating over a Maildir mailbox instantiated with default settings, the 
Mailbox class will return self._factory(self.get_file(key)), leaking the file 
descriptor returned by get_file(). Thus, iterating over any reasonably sized 
maildir mailbox will cause file descriptors to be leaked, and if a 
non-refcounting GC is used (as in PyPy), it is very likely that the process 
will run out of file descriptors.

I see that the default has changed to None, for Py3k, but this still means that 
using a message factory unavoidably leaks file descriptors. Ideally, I'd like 
the Mailbox class to close the file descriptor passed into the factory; this 
would mean that file descriptors are never leaked during iteration.

--
components: Library (Lib)
messages: 132988
nosy: moyix
priority: normal
severity: normal
status: open
title: Maildir iterator leaks file descriptors by default
type: behavior
versions: Python 2.5, Python 2.6, Python 2.7

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11767
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com