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.

The OP would accept a documentation change if the code's not going to be changed.

I am going to fix it so that it raises the NNTPConnectionError rather than update the documentation.

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.

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.

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?

That's a good point. I can add that so the NNTPConnectError can inherit the EOFError

Here is an updated patch.

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.

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

I'ved changed the comment to say Connection closed unexpectedly.

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

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

PPS: Documentation should probably have the “New in version 3.5” tag as well

Thank yo so much, Martin! I will incorporate these changes and add them soon!

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

nntplib is deprecated as per PEP 594, so there won't be further enhancements to it.

nosy: + iritkatriel
messages: + msg415082

resolution: wont fix
stage: patch review -> resolved

messages: + msg224794

messages: + msg222963

messages: + msg222958
versions: + Python 3.5, - Python 3.2

messages: + msg114494
versions: + Python 3.2, - Python 2.7