classification
Title: nntplib shouldn't raise generic EOFError
Type: enhancement Stage: patch review
Components: Library (Lib) Versions: Python 3.5
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Ankur.Ankan, Lita.Cho, berker.peksag, ezio.melotti, jesstess, martin.panter, mattrope, pitrou
Priority: low Keywords: needs review, patch

Created on 2005-04-20 19:52 by mattrope, last changed 2015-06-22 02:19 by martin.panter.

Files
File name Uploaded Description Edit
nntplib_error.patch Lita.Cho, 2014-07-10 23:04
nntplib_error.patch Lita.Cho, 2014-07-13 18:39
nntplib_error_v2.patch Lita.Cho, 2014-08-05 02:11 review
Messages (16)
msg25090 - (view) Author: Matt Roper (mattrope) Date: 2005-04-20 19:52
Python's nntplib currently raises a generic EOFError if
the connection is closed unexpectedly.  This seems
inconsistent with other Python libraries (smtplib,
imaplib, etc.) and is unexpected behaviour.  It seems
that a new Exception class derived from the NNTPError
(e.g., NNTPConnectionError) should be used instead.

As it stands now, the only indication that EOFError can
be raised is in the docstring for the internal
getline() method.  There is no mention in the
documentation that higher level methods call getline()
and can raise the EOFError to the application level. 
If no new exception class is added for this situation,
it would be nice to have this behaviour noted in the
documentation.
msg114494 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-08-21 13:55
The OP would accept a documentation change if the code's not going to be changed.
msg222685 - (view) Author: Lita Cho (Lita.Cho) * Date: 2014-07-10 18:24
I am going to fix it so that it raises the NNTPConnectionError rather than update the documentation.
msg222711 - (view) Author: Lita Cho (Lita.Cho) * Date: 2014-07-10 23:04
I have a fix and added some test coverage in order to make sure the NNTFConnectError was being called. However, in the test case, I am monkey patching. If there is a way to do this with mock, I would appreciate the feedback.
msg222958 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2014-07-13 17:56
diff -r 8f85262fbe8a Lib/nntplib.py
--- a/Lib/nntplib.py	Sun Jul 06 02:24:24 2014 -0400
+++ b/Lib/nntplib.py	Thu Jul 10 16:10:38 2014 -0700
@@ -122,6 +122,9 @@
     """Error in response data"""
     pass
 
+class NNTPConnectError(NNTPError):
+    """Error during connection establishment."""
+    pass

Could you also document the new exception? (See Doc/library/nntplib.rst and please add a versionadded directive)

The pass statement is redundant, but we could keep it to be consistent with rest of the library.


@@ -435,7 +438,7 @@
             raise NNTPDataError('line too long')
         if self.debugging > 1:
             print('*get*', repr(line))
-        if not line: raise EOFError
+        if not line: raise NNTPConnectError()

    if not line:
        raise NNTPConnectError

looks more readable to me.

Also, you could add a more descriptive error message.
msg222960 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2014-07-13 18:04
Wouldn't this be backward incompatible?
Even if the EOFError that is raised is not documented explicitly, people might be catching it, and switching to a new exception would break their programs.  Maybe NNTPConnectError should inherit from EOFError too?
msg222961 - (view) Author: Lita Cho (Lita.Cho) * Date: 2014-07-13 18:07
That's a good point. I can add that so the NNTPConnectError can inherit the EOFError
msg222963 - (view) Author: Lita Cho (Lita.Cho) * Date: 2014-07-13 18:39
Here is an updated patch.
msg224670 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2014-08-04 01:50
I could be wrong, but isn’t this error raised when expecting a response from any command, not just during “connection establishment”? Perhaps change the docstring to say something like “Connection closed unexpectedly” instead.
msg224725 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-08-04 14:18
Indeed, I find the description rather vague and potentially misleading. "Connection establishment" would usually refer to the TCP connection, but an EOFError is actually a high-level, logical error (it has nothing to do with networking per se: it's probably more of a server failure - or a client bug perhaps :-)).
msg224794 - (view) Author: Lita Cho (Lita.Cho) * Date: 2014-08-05 02:11
I'ved changed the comment to say Connection closed unexpectedly.
msg225634 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2014-08-22 01:47
NNTPConnectError does still seem a slightly awkward name. I would go for NNTPConnectionError instead, but I’m also happy to put my bikeshed paint away let this patch be applied as is :)

Handling of NNTPTemporaryError with a code of 400 is similar to handling of this EOFError. But I guess there is not much you could do with the API unless you made a separate subclass for 400 errors (like all the new EnvironmentError/OSError subclasses), which would be rather severe. My current workaround looks a bit like this:

try:
    [_, info] = nntp.body(...)
except NNTPTemporaryError as err:
    [code, *msg] = err.response.split(maxsplit=1)
    if code != "400":
        raise
except EOFError:  # Or NNTPConnect(ion)Error
    msg = ()
else:
    break  # Handle successful response
[msg] = msg or ("Server shut down connection",)
# Handle connection shutdown by server
msg225638 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2014-08-22 02:48
Some more points:

* I suggest adding something like this to the documentation:

exception nntplib.NNTPConnect[ion]Error
  Exception raised when the server unexpectedly shuts down the connection.

* The tests should use BytesIO rather than StringIO. Other than that, I think monkey-patching the file attribute in the tests is fine; that’s probably the way I would do it!

* The new exception should be added to __all__.
msg225640 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2014-08-22 03:04
PPS: Documentation should probably have the “New in version 3.5” tag as well
msg225641 - (view) Author: Lita Cho (Lita.Cho) * Date: 2014-08-22 03:13
Thank yo so much, Martin! I will incorporate these changes and add them soon!
msg245612 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-06-22 02:19
Quick survey of other communication protocol modules:

ftplib: uses EOFError; not documented
http.client: custom IncompleteRead exception
imaplib: custom IMAP4.abort exception
poplib: custom error_proto exception
smtplib: SMTPServerDisconnected exception, subclassing OSError
telnetlib: EOFError is documented
History
Date User Action Args
2015-06-22 02:19:38martin.pantersetmessages: + msg245612
2014-08-22 03:13:11Lita.Chosetmessages: + msg225641
2014-08-22 03:04:22martin.pantersetmessages: + msg225640
2014-08-22 02:48:00martin.pantersetmessages: + msg225638
2014-08-22 01:47:42martin.pantersetmessages: + msg225634
2014-08-05 02:11:38Lita.Chosetfiles: + nntplib_error_v2.patch

messages: + msg224794
2014-08-04 14:18:18pitrousetnosy: + pitrou
messages: + msg224725
2014-08-04 01:50:38martin.pantersetnosy: + martin.panter
messages: + msg224670
2014-07-13 18:39:13Lita.Chosetfiles: + nntplib_error.patch

messages: + msg222963
2014-07-13 18:07:14Lita.Chosetmessages: + msg222961
2014-07-13 18:04:55ezio.melottisetnosy: + ezio.melotti
messages: + msg222960
2014-07-13 17:56:23berker.peksagsetnosy: + berker.peksag

messages: + msg222958
versions: + Python 3.5, - Python 3.2
2014-07-13 17:21:00jesstesssetkeywords: + needs review
stage: test needed -> patch review
2014-07-10 23:04:17Lita.Chosetfiles: + nntplib_error.patch
keywords: + patch
messages: + msg222711
2014-07-10 18:24:15Lita.Chosetnosy: + jesstess, Lita.Cho
messages: + msg222685
2014-02-03 18:39:27BreamoreBoysetnosy: - BreamoreBoy
2013-03-08 04:53:08Ankur.Ankansetnosy: + Ankur.Ankan
2010-08-21 13:55:56BreamoreBoysetnosy: + BreamoreBoy

messages: + msg114494
versions: + Python 3.2, - Python 2.7
2009-02-16 01:01:26ajaksu2setstage: test needed
versions: + Python 2.7
2005-04-20 19:52:50mattropecreate