I'll send a patch that updates/fixes handling of
file closes in the internal proxy-file classes.
It could cause errors yet because self._file is
del-eted but that field may still be used afterwards.


>>> mb = mailbox.Maildir('sdaoden', create=False)
>>> mbf = mb.get_file(mb.keys()[0])
>>> msg = email.parser.BytesParser().parse(mbf, headersonly=True)
>>> mbf.close()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/steffen/usr/opt/py3k/lib/python3.3/mailbox.py", line 1922, in close
    if hasattr(self._file, 'close'):
AttributeError: '_ProxyFile' object has no attribute '_file'


The patched version will always act correctly.
And yes, i'll open yet another issue due to the
email.parser (or even TextIOWrapper) based problem.

Here's the patch.

mbf.close() should not fail when called twice. The close() method in the io module states that "This method has no effect if the file is already closed."
But then, is "close=False" necessary?

On Mon, Mar 28, 2011 at 09:52:43PM +0000, Amaury Forgeot d'Arc wrote:
> But then, is "close=False" necessary?

It's about 'class _PartialFile(_ProxyFile)', for which this 
argument is always false. 
Alternatively there could be a "protected" _ProxyFile._do_close() 
method which would only be used by the _PartialFile subclass; 
then the ctor argument could be dropped in favour of that. 
I've gone that way because both classes are internal.
?

This new patch adheres your suggestion.
Now all implemented operations perform
a "file is open at all" check and raise
ValueError if not, which somewhat reflects
what i've seen when i was looking into fileio.c.

My questions:
- shouldn't _ProxyFile inherit from
  a real Python I/O class, for example IOBase?
  Like this it would stand in a line where it belongs.
  I could do that (not much missing for that).
- I have no idea of the test framework.  The last hour
  i tried to adjust test_mailbox.py a bit, but it's hard.
  It's not about design and it's not about algorithm.  Uff.
  I could nonetheless add some cases to yet existing
  test functions and change another one.  But this needs
  a real eye on it!
  (For example: is there a AssertNoRaises?)

Given your problem report wouldn't the simplest solution be to change the close method to be:

   if hasattr(self, '_file'):
       if hasattr(self._file, 'close'):
           self._file.close()
       del self._file

As for a test, it seems to me that adding a test to the appropriate existing cases that calls get_file and then closes the returned object twice should be sufficient.

On Thu, Apr 07, 2011 at 01:12:46AM +0000, R. David Murray wrote:
> [...] should be sufficient.

It is sufficient to fix the resource warning.
Having a completely dynamic language is a nice thing.
I would not do it like that.

Instead i would even consider to provide at least ProxyFile 
publically (i.e. in I/O), because it is a nifty thing which may 
be useful for a number of applications. 
(PartialFile is probably too complicated in respect to SMP and FD 
sharing to be public as a generic (non-abstract) class.) 

I don't want to come up with a C++ lib again, but there Device-s 
are doubly-linked (spinlock protected), and each has a list of 
attached Stream-s, and upon program exit the list(s) is/are 
walked:

    open:
	"IO::Device::open(): already open!!%@"
		"\tI'll fail with Errno::ebusy now.%@"
    close:
	"IO::Device::close(): device *not* open!!%@"
		"\tThis may break non-debug enabled code...%@"
		"\tI'll fail with Errno::enodev now.%@"

	"IO::Device::close(): error occurred while closing one%@"
		"\tof the still attached streams!%@"
    ~Device:
	"IO::Device::~Device(): still isOpen()!%@"
		"\tEither subclass::close() does not call Device::close(),%@"
		"\tor it's destructor does not check the open state.%@"
		"\tI'll clean up anyway. Expect crashes soon.%@"

Stream somewhat likewhat.  The Issues #11767, #11466, #11701, to 
name a few, would never happen there if at least one test uses at 
least one of the involved classes once.

Now this is not true for Python's I/O, but i think that the 
interface should at least be complete and act as documented, 
so saying "file-like object" implies raising of ValueError if 
an object is closed etc. 
Even if the class is a somewhat hidden implementation detail. 

So of course hasattr() could be used instead of a special member 
to test for is_open().  (I hope it's slower than the latter.) 
There is no IOXY.open(), so, well, ok.

By the way, mailbox.py also has some other pitfalls as far as 
i remember; i think i've even seen calls to _lock_file() for 
a successful is_locked() (self._locked or so) state, which results 
in unlocking because of cleanup due to lock-taking failure; 
it was a shallow look only. 
(That is, because: rescue me from here!!!)

I'll attach 11700.yeah.diff, which inherits from RawIOBase; 
it's test adds some stuff naively. 
Note this is a few-hours hack in an area i haven't touched that 
much yet; it's a bit mysterious which I/O base class implements 
which functions or expects which function to be present in 
a subclass ...  And which raises what and when. 
That is - i would need to regulary review that at least once 
before i would ship that out for real. 
And i'll attach 11700.sigh.diff, which does only what is 
necessary, following your suggestions.

Now this: choose the right diff and i'll implement #11783. 
:)

I don't understand what you are saying about raising a ValueError on close.  f = open('x'); f.close(); f.close() does not raise any error, as Amaury pointed out.

So I still don't understand the motivation for a more complex fix.

On Thu, Apr 07, 2011 at 04:41:52PM +0000, R. David Murray wrote:
> 
> R. David Murray <rdmurray@bitdance.com> added the comment:
> 
> I don't understand what you are saying about raising a ValueError on close.
> f = open('x'); f.close(); f.close() does not raise any error, as Amaury pointed out.
> 
> So I still don't understand the motivation for a more complex fix.

Now i've indeed looked into io.rst and i've found this:

:class:`IOBase` provides these data attributes and methods:

.. method:: close()

  Flush and close this stream. This method has no effect if the file is
  [Mojo Risin', gotta Mojo Risin']
  already closed. Once the file is closed, any operation on the file
  (e.g. reading or writing) will raise a :exc:`ValueError`.
  [I gotta, wooo, yeah, risin']
  As a convenience, it is allowed to call this method more than once;
  only the first call, however, will have an effect.

And a minute ago i've also done this:

...     def __init__(self):
...             pass
... 
>>> dir(y)

and i've found out that i should have done that first, but i'm 
still surprised how easy Python is - 'am waiting for 
'as -o mb.o mailbox.py' to produce nice x86 pseudo machine code??

So i will reimplement yeah.diff even more fancy tomorrow, 
and (urgh!) add more tests for the new input functions. 
(I'll continue to discontinue support for read1().) 
That's what i will do.
Good night.

(Hrmpf, it seems a '>>> class y(io.RawIOBase):' line has been 
swallowed in at least Roundup.)

So here is the rewritten .yeah-2.diff. 
It drops the ._trackpos stuff once again due to problems with 
position tracking in case of failures, i.e. to go that path to the 
end a whole bunch of try..catch would have been necessary. 
So this is very expensive but let's hope the OS VFS is smarter 
than we are so that this ends up as two mostly no-op syscalls. 
:-(

I added more tests (i'm absolutely convinced that the tests i've 
found in test_mailbox.py really find all the cutting edges <;). 
On my box this is clean. 
(It's again a rewrite so it needs at least another review before 
i would ship that out, on the other hand.)

..
> So here is the rewritten .yeah-2.diff. 
..
> I added more tests (i'm absolutely convinced that the tests i've 
> found in test_mailbox.py really find all the cutting edges <;). 
> On my box this is clean. 

Haha, now this is *very* funny!

______
Traceback (most recent call last):
...
  File "/Users/steffen/usr/opt/py3k/lib/python3.3/email/parser.py", line 104, in parse
    return self.parser.parse(fp, headersonly)
  File "/Users/steffen/usr/opt/py3k/lib/python3.3/mailbox.py", line 1900, in flush
    raise io.UnsupportedOperation('flush')
Exception: io.UnsupportedOperation: flush
______

So, don't ask me why anyone tries to flush a readonly ProxyFile! 
BytesParser wraps this in TextIO, and in Modules/_io/textio.c i find:

c_STRVAR(textiowrapper_doc,
    "\n"
    "If line_buffering is True, a call to flush is implied when a call to\n"
    "write contains a newline character."
    );
..
typedef struct
{
..
    /* Reads and writes are internally buffered in order to speed things up.
       However, any read will first flush the write buffer if itsn't empty.
..
} textio;

No reason to call flush. 
So i replaced flush() and got this:

______
...
  File "/Users/steffen/usr/opt/py3k/lib/python3.3/email/parser.py", line 104, in parse
    return self.parser.parse(fp, headersonly)
  File "/Users/steffen/usr/opt/py3k/lib/python3.3/email/parser.py", line 48, in parse
    data = fp.read(8192)
Exception: AttributeError: '_PartialFile' object has no attribute 'read1'
______

Nice.  It seems that deriving _ProxyFile from io.RawIOBase will 
not work correctly with the current EMail package because 
BytesParser uses TextIOWrapper expects io.BufferedIOBase. 
Regardless of the fact i think TextIOWrapper should be 
configurable not to close the "buffer" - s...!

Anyway, it's seems not to be practical to implement ProxyFile 
*correctly*.  Therefore i'll attach yeah-3.diff which falsely 
ignores calls to flush(). 
To make this s... rock it is now also derived from 
io.BufferedIOBase, thus i've reintroduced read1() which - true - 
is now also tested!  *YES IT CAN*!
Nice weekend.

I reviewed this.
And moved a _PartialFile-only _read() case to _PartialFile where
it belongs (*this* _ProxyFile will never be extended to stand
alone so i shouldn't have moved that the other direction at all).

Amaury Forgeot d'Arc wrote:
> mbf.close() should not fail when called twice. The close() method in the > io module states that "This method has no effect if the file is already > closed."
> But then, is "close=False" necessary?

I see you've detached.

Here's a patch that fixes the reported bug (calling close twice fails with an AttributeError) the simple way.

Note that there was actually a test for the buggy behavior, which is rather odd considering that there is also a 'closed' method that would fail similarly if close was ever called.  (The only use for the existing 'closed' method that I can see is to see if somebody else closed the file out from under the ProxyFile, a 'feature' that seems of dubious utility.)

It seems clear to me that calling close more than once should be legal, so I fixed the test.  closed will still report if the underlying file has been closed out from under ProxyFile.

Steffen, I still don't understand what you are trying to achieve with your patches.  I plan to close this issue by applying my patch.

On Sat, Apr 16, 2011 at 08:17:30PM +0000, R. David Murray wrote:
> [...] rather odd considering that there is also a 'closed'
> method that would fail similarly if close was ever called.

Maybe someone got not enough feedback after she wrote the patch.

> (The only use for the existing 'closed' method that I can see is
> to see if somebody else closed the file out from under the
> ProxyFile, a 'feature' that seems of dubious utility.)

After i've read io.rst - as above - i thought someone started to
comply to one of the interfaces shown therein, went away because
the current bug was fixed and there was no more time left at the
moment to continue the work ... and actually never found back to
do so.  This really made sense to me because that actually happened
a few days after i first got on the Internet to contact python.org.

I would like to say that if someone reads "file-like objects" in
io.rst and then also in mailbox.rst, and uses a box.get_file()
returned object with that description in mind, there will be
inconsistent failures.

> Steffen, [...] I plan to close this issue by applying my patch.

That is a pity.  But the main thing is that the bug gets fixed.

- .closed is better than my version (talking about .sigh).
- I would add a self.assertFalse(proxy.closed) test before the
  first .close().

Ah.  Well, since the io module and its classes didn't exist when that code in mailbox.py was written, no, that's not what happened :)

Nor does 'file like object' in Python necessarily mean conformance to the io specification.  We are *tending* in that direction, but the actual expected method set is really considerably smaller (and I'm not sure it is documented anywhere).

I will incorporate your suggestion into the patch, thanks.  Ezio also pointed out that I should be checking for the 'closed' method's existence before calling it, so I will add that as well.

Updated patch addressing Stefen and Ezio's comments.

Hello, David, i'm about to remind you that this issue is still
open (and the next release is about to come soon).
I think we do agree at least in the fact that this is a bug :).
Well, your mailbox_close_twice.patch no. 2 still imports on the
current tip.

(I'm still a bit disappointed that you don't want to -a-r-m-
upgrade the proxies to the full implementation i've posted.  But
it's ok.  By the way: you're the first american i know who doesn't
want to upgrade his arms!  And i do have had an ex-uncle who is
a fellow countryman of yours.)

Regards from Germany during kitschy pink sunset

New changeset b89d193cbca5 by R David Murray in branch '2.7':
#11700: proxy object close methods can now be called multiple times
http://hg.python.org/cpython/rev/b89d193cbca5

New changeset 8319db2dd342 by R David Murray in branch '3.2':
#11700: proxy object close methods can now be called multiple times
http://hg.python.org/cpython/rev/8319db2dd342

New changeset 0705b9037b20 by R David Murray in branch 'default':
merge #11700: proxy object close methods can now be called multiple times
http://hg.python.org/cpython/rev/0705b9037b20