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.

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.

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.

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?

How about this? Though I have no idea, how a test could be created for this. If someone could advise me on that, I'll be happy to write tests/work further on the patch.

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.

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.

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.

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.

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

New changeset fea1920ae75f by R David Murray in branch '3.2':
bpo-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 bpo-11767: use context manager to close file in __getitem__ to prevent FD leak
http://hg.python.org/cpython/rev/1d7a91358517

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

Should I try to do something about this right now or should I wait until bpo-1170 is finished&closed and only then try to fix this issue too?

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

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.

Python 2.7 is no longer supported.