| msg132395 - (view) |
Author: Steffen Daode Nurpmeso (sdaoden) |
Date: 2011-03-28 12:26 |
| 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 "", line 1, in 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. |
|
|
| msg132396 - (view) |
Author: Steffen Daode Nurpmeso (sdaoden) |
Date: 2011-03-28 12:29 |
| Here's the patch. |
|
|
| msg132427 - (view) |
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) *  |
Date: 2011-03-28 21:52 |
| 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? |
|
|
| msg132479 - (view) |
Author: Steffen Daode Nurpmeso (sdaoden) |
Date: 2011-03-29 11:39 |
| 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. ? |
|
|
| msg132502 - (view) |
Author: Steffen Daode Nurpmeso (sdaoden) |
Date: 2011-03-29 19:11 |
| 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?) |
|
|
| msg133187 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2011-04-07 01:12 |
| 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. |
|
|
| msg133221 - (view) |
Author: Steffen Daode Nurpmeso (sdaoden) |
Date: 2011-04-07 15:22 |
| 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. :) |
|
|
| msg133230 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2011-04-07 16:41 |
| 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. |
|
|
| msg133255 - (view) |
Author: Steffen Daode Nurpmeso (sdaoden) |
Date: 2011-04-07 20:41 |
| 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. |
|
|
| msg133324 - (view) |
Author: Steffen Daode Nurpmeso (sdaoden) |
Date: 2011-04-08 16:05 |
| (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.) |
|
|
| msg133385 - (view) |
Author: Steffen Daode Nurpmeso (sdaoden) |
Date: 2011-04-09 13:27 |
| .. > 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. |
|
|
| msg133476 - (view) |
Author: Steffen Daode Nurpmeso (sdaoden) |
Date: 2011-04-10 17:44 |
| 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). |
|
|
| msg133660 - (view) |
Author: Steffen Daode Nurpmeso (sdaoden) |
Date: 2011-04-13 11:31 |
| 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. |
|
|
| msg133906 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2011-04-16 20:17 |
| 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. |
|
|
| msg133930 - (view) |
Author: Steffen Daode Nurpmeso (sdaoden) |
Date: 2011-04-17 15:39 |
| 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(). |
|
|
| msg133931 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2011-04-17 15:57 |
| 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. |
|
|
| msg133939 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2011-04-17 22:22 |
| Updated patch addressing Stefen and Ezio's comments. |
|
|
| msg136783 - (view) |
Author: Steffen Daode Nurpmeso (sdaoden) |
Date: 2011-05-24 19:26 |
| 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 |
|
|
| msg138565 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2011-06-18 02:26 |
| 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 |
|
|