[issue22350] nntplib file write failure causes exception from QUIT command
Martin Panter added the comment: For the record, the SMTP scenario was Issue 17498, where code that is about to raise an exception attempts an RSET command that could also fail. I do think each change in my patch is essentially the same case: restoring the invariant expected by the __exit__() method, that either the protocol state allows QUIT, or there is no connection. But I agree it may help divide this into smaller pieces. I am uploading getlongresp-loop.patch, which fixes just the receiving loop in _getlongresp(). I have also added some logic to avoid errors raised when flushing and closing the socket do not mask the original exception, which I understand David was concerned about. I guess it is possible (though unlikely) that an EPIPE or ECONNRESET flushing the send buffer could mask a KeyboardInterrupt raised inside the loop. Sorry I took a while to follow up on this, but please have a look and let me know if this simplified patch makes any sense. -- Added file: http://bugs.python.org/file39490/getlongresp-loop.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22350 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22350] nntplib file write failure causes exception from QUIT command
R. David Murray added the comment: That's fine, but you need to look at each case individually, and not try to deal with them as if they were all the same. This is because you want the correct error to percolate up. For example, in smtplib we have a case where the server may respond to a command by closing the connection (which is technically a violation of the RFC). It used to be that smtplib would raise a connection error at that point because it would try to send an RST command to the server over the closed connection. The original error message reported by the server was lost. The solution was to rewrite the error handling so that we had an internal send_RST that was different from the one that would be used by the application, and that internal RST send was wrapped in a try/except ignoring the connection error. That way the command the application called returned the received response, and then the application got the connection closed error the *next* time it tried to use the connection. nntplib presumably requires as a similar strategy if the network connection terminates unexpectedly: just ignoring the connection closed error when the quit is sent. The code already closes the connection after the QUIT attempt, so that should leave things in the correct state for all other errors. Other network errors may benefit from additional handling, but I don't know since I don't have examples here to think about :) As for the keyboard interrupt problem, it looks like what needs to happen is a custom response handler for the internal QUIT, that will recognize when it is not getting its response back and just return. This is more similar to the smptlib problem than I originally thought: it seems like the basic problem is that that internal QUIT needs custom handling that is different from the QUIT issued at the application level. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22350 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22350] nntplib file write failure causes exception from QUIT command
Martin Panter added the comment: Serhiy, regarding _getresp() calling resp.decode(): You are probably right that a failure decoding the response line is recoverable in some circumstances, such a GROUP command. But in other cases, say an ARTICLE command, the response won’t have been fully drained, so the connection is no longer usable. I guess I could shift the error handling to a higher level if you think it is necessary. Regarding the workaround in my original post: It is not a good solution, because it is still trying to send a QUIT request and wait for a response in the error handler. If you are trying to interrupt a hanging network connection for example, you have to hit Ctrl+C twice, or wait a long time for the QUIT command to get some invalid data and raise its confusing protocol error. David: The error handling inside _getlongresp() is the main place that concerned me. But as I made the changes there I identified the other places I thought could raise unexpected exceptions (network errors) or block (therefore a KeyboardInterrupt is likely). Draining the response might be sensible for handling an error writing to the file in some cases. My /dev/full example was actually meant to be a simple example of more complicated exceptions, such as KeyboardInterrupt, network timeouts, or if the programmer wants to peek at the start of a large message and then abort the download. So it seems my simplified test case was misleading, because in these other cases, draining the response isn’t so appropriate. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22350 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22350] nntplib file write failure causes exception from QUIT command
Martin Panter added the comment: Well with a regular BufferedWriter, the chained exception is the same type as the original exception, so it does not really matter. I was just using the out-of-space exception as an easy way to get the body() method to abort in the middle of a command. The problem with the NNTP library is that the NNTP class assumes that it can send a QUIT command when it shouldn’t be. The result can be a giant error message full of apparent garbage data, rather like in Issue 21468. The minimum behaviour that I expect is that when the context manager exits, it should not cause a double exception with a different error type. My patch closes the connection as soon as an unrecoverable error happens, so __exit__() will not send a QUIT command and receive an invalid response. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22350 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22350] nntplib file write failure causes exception from QUIT command
Serhiy Storchaka added the comment:
But the same issue exists with regular files.
with open('/dev/full', 'wb') as f:
... f.write(b'x'*1)
...
Traceback (most recent call last):
File stdin, line 2, in module
OSError: [Errno 28] No space left on device
And using Python implementation for detailed traceback:
import _pyio
with _pyio.open('/dev/full', 'wb') as f:
... f.write(b'x'*1)
...
Traceback (most recent call last):
File stdin, line 2, in module
File /home/serhiy/py/cpython/Lib/_pyio.py, line 1184, in write
self._flush_unlocked()
File /home/serhiy/py/cpython/Lib/_pyio.py, line 1211, in _flush_unlocked
n = self.raw.write(self._write_buf)
OSError: [Errno 28] No space left on device
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File stdin, line 2, in module
File /home/serhiy/py/cpython/Lib/_pyio.py, line 451, in __exit__
self.close()
File /home/serhiy/py/cpython/Lib/_pyio.py, line 767, in close
self.flush()
File /home/serhiy/py/cpython/Lib/_pyio.py, line 1204, in flush
self._flush_unlocked()
File /home/serhiy/py/cpython/Lib/_pyio.py, line 1211, in _flush_unlocked
n = self.raw.write(self._write_buf)
OSError: [Errno 28] No space left on device
What behavior you expect?
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22350
___
___
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22350] nntplib file write failure causes exception from QUIT command
Serhiy Storchaka added the comment: I doubt that it is good idea to close connection on *any* error. For example in _getresp() resp.decode() can raise an exception, but this failure isn't related to the ability of sending QUIT and other commands. Sample error handling in your first message looks more reliable. Set special flags after failing command (but only if error is related to the connection), and ignore QUIT errors if it is set. After successful command this flag should be reset. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22350 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22350] nntplib file write failure causes exception from QUIT command
R. David Murray added the comment: It looks to be clearly the case that there needs to be some error handling code wrapped around the file write in _getlongresp. Your patch doesn't do just that, however, so it seems like there may be other errors you are also worried about? Can you explain their nature? They should be dealt with separately from the errors writing to file issue, though, which is a special case. And that case should be dealt with by draining the response before returning. -- nosy: +r.david.murray ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22350 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22350] nntplib file write failure causes exception from QUIT command
Changes by Serhiy Storchaka storch...@gmail.com: -- assignee: - serhiy.storchaka nosy: +serhiy.storchaka ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22350 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22350] nntplib file write failure causes exception from QUIT command
Changes by Jakub Wilk jw...@jwilk.net: -- nosy: +jwilk ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22350 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22350] nntplib file write failure causes exception from QUIT command
Changes by Serhiy Storchaka storch...@gmail.com: -- nosy: +pitrou stage: - patch review type: - behavior versions: +Python 2.7, Python 3.5 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22350 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22350] nntplib file write failure causes exception from QUIT command
Martin Panter added the comment: Here is a patch with a fix and a test -- keywords: +patch Added file: http://bugs.python.org/file37501/fail-close.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22350 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22350] nntplib file write failure causes exception from QUIT command
New submission from Martin Panter:
The following code triggers an NNTPProtocolError, as long as the body is large
enough to cause an intermediate flush of the output file. The reason I think is
that the body() method aborts in the middle of reading the BODY response, and
when the context manager exits, a QUIT command is attempted, which continues to
read the BODY response.
with NNTP(localhost) as nntp:
... nntp.body(example, file=/dev/full)
...
Traceback (most recent call last):
File /usr/lib/python3.4/nntplib.py, line 491, in _getlongresp
file.write(line)
OSError: [Errno 28] No space left on device
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File stdin, line 2, in module
File /usr/lib/python3.4/nntplib.py, line 757, in body
return self._artcmd(cmd, file)
File /usr/lib/python3.4/nntplib.py, line 727, in _artcmd
resp, lines = self._longcmd(line, file)
File /usr/lib/python3.4/nntplib.py, line 518, in _longcmd
return self._getlongresp(file)
File /usr/lib/python3.4/nntplib.py, line 504, in _getlongresp
openedFile.close()
OSError: [Errno 28] No space left on device
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File stdin, line 2, in module
File /usr/lib/python3.4/nntplib.py, line 367, in __exit__
self.quit()
File /usr/lib/python3.4/nntplib.py, line 936, in quit
resp = self._shortcmd('QUIT')
File /usr/lib/python3.4/nntplib.py, line 512, in _shortcmd
return self._getresp()
File /usr/lib/python3.4/nntplib.py, line 459, in _getresp
raise NNTPProtocolError(resp)
nntplib.NNTPProtocolError: line of data from BODY command
It is hard to work around because the context manager and quit() methods seem
to be the only public interfaces for closing the connection, and they both try
to do a QUIT command first. I am thinking of something equivalent to this for a
workaround, however it is bit hacky and may not be reliable in all cases:
nntp = NNTP(localhost)
abort = False
try:
...
try:
nntp.body(example, file=/dev/full)
except (NNTPTemporaryError, NNTPPermanentError):
raise # NNTP connection still intact
except:
abort = True
raise
...
finally:
try:
nntp.quit()
except NNTPError:
# Connection cleaned up despite exception
if not abort:
raise
Perhaps the “nntplib” module could abort the connection itself if any command
does not complete according to the protocol. Or at the very least, provide an
API to manually abort the connection without poking at the internals.
--
components: Library (Lib)
messages: 226526
nosy: vadmium
priority: normal
severity: normal
status: open
title: nntplib file write failure causes exception from QUIT command
versions: Python 3.4
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22350
___
___
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
