smtplib.SMTP() raises socket.error rather than SMTPConnectError

just try this on a non-responding address

>>> srv=smtplib.SMTP('192.168.13.22')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "c:\python25\lib\smtplib.py", line 244, in __init__
    (code, msg) = self.connect(host, port)
  File "c:\python25\lib\smtplib.py", line 311, in connect
    (code, msg) = self.getreply()
  File "c:\python25\lib\smtplib.py", line 352, in getreply
    line = self.file.readline()
  File "C:\Python25\lib\socket.py", line 346, in readline
    data = self._sock.recv(self._rbufsize)
socket.error: (10054, 'Connection reset by peer')

Why is this a bug? Do the docs promise that smtplib only raises
SMTPConnectError?

Not in so many words, but yes it does

I cite:

exception SMTPException 
Base exception class for all exceptions raised by this
module
...
exception SMTPConnectError 
Error occurred during establishment of a connection
with the server. 
...

Note the word "all". So, at least one should be able
to catch SMTPException, but it is not the case

Just my 2 cents. I do believe it is a great piece of
work.

Regards

--- Christian Heimes <report@bugs.python.org> wrote:

> 
> Christian Heimes added the comment:
> 
> Why is this a bug? Do the docs promise that smtplib
> only raises
> SMTPConnectError?
> 
> ----------
> nosy: +tiran
> priority:  -> normal
> 
> __________________________________
> Tracker <report@bugs.python.org>
> <http://bugs.python.org/issue2118>
> __________________________________
> 

      ____________________________________________________________________________________
Be a better friend, newshound, and 
know-it-all with Yahoo! Mobile.  Try it now.  http://mobile.yahoo.com/;_ylt=Ahu06i62sR8HDtDypao8Wcj9tAcJ

I see! You are right. Either the docs or the code need an update

It seems the right thing to do would be to have it raise a base
exception, but SMTPConnectError docstring states "Error during
connection establishment.", so I chosen to use it with the errno and
message from socket.error, even if it's supposed to happen only after
connection is already stablished, since it's a subclass of
SMTPResponseException.

As a bonus I removed a explicit raise socket.error from line 290, when
an user pass a non-integer port.

Previous patch didn't passed the tests right. This patch fixes both the
code, unindenting port number conversion to integer and the test.

Verified Werneck's patch and it also works on 2.6 and 3.0.

However, the previous code used to present a "friendly" message about
non-numeric ports: "socket.error: nonnumeric port" and now it raises
"ValueError: invalid literal for int() with base 10".

Should it be changed to inform that the exception is due to a
non-numeric port? (Just wrap int(port) with a try and change the raised
exception)

I applied the patch to test_smtplib.py only and all tests passed, surely that's not correct.

Removing the explicit test for a non-numeric port in smtplib.py seems strange to me, could someone please explain the rationale behind this.

I also noticed this line in the patch.

if self.debuglevel > 0: print>>stderr, "connect:", msg

And this comment in set_debuglevel in smtplib.py

"A non-false value results in debug messages for connection and for all
messages sent to and received from the server."

IIRC if a string was passed into set_debuglevel the comparison would fail in py3k, am I correct?  The print statement would not work in py3k.
Given that the debuglevel code obviously doesn't get tested perhaps it should simply be stripped from the code?

I've uploaded a patch for 3.2 that throws a ValueError exception for non numeric port numbers and SMTPSocketConnectError for socket connection failures. 

This patch introduces an API change, by creating the SMTPSocketConnectError which provides information about the socket error. The API documentation will therefore need to be updated if this patch is accepted.

A little backstory: the patch was created during the PyOhio sprint, and as we recreated the patch for py3k we noticed that SMTPConnectError is providing an *SMTP* error code.  The gaierrors are obviously not SMTP errors.  To allow the agreed upon semantics (catching SMTPConnectError catches errors resulting from attempting to establish a session), we created a subclass of SMTPConnectError called SMTPSocketConnectError that has errno and strerror attributes instead of smtp_code and smtp_error. The giaerror errno is not an SMTP error code, and can not be guaranteed to be a disjoint set of number, nor is there an SMTP error code that we can return and then put the errno in an appropriate message.  Obviously code that inspects smtp_code will need to handle this subclass specially, but since previously it had to handle the giaerror separately this does not seem like a big issue.  Code that only cares about catching the error and possibly using the str of the error does not need to care.

An alternative to this patch would be a doc fix patch that changed the wording to indicate that it is not *all* exceptions that are caught (which certainly isn't true...we raise ValueError for an invalid port number, and that seems correct), but rather all *SMTP protocol* related exceptions.  However, it does seem very convenient to be able to just catch SMTPConnectError in order to catch any errors resulting from an attempt to establish a connection using valid parameters.

Two further things to note: neither this patch nor the original patch actually fix the bug reported by the OP, nor does the test code test it.  In the original report it is getreply that is raising the error, and the patches do not wrap getreply in the try/except.  Because testing this case appears non-trivial, we elected not to tackle it, but just reimplemented the original patch, which does catch one class of socket related connect errors.  If the subclassed-error approach is accepted, the patch can be expanded to wrap the getreply call and test it as well.

The second thing is a response to Breamorboy: the debug code does indeed have slightly different semantics in Python3 (good catch), but that's a separate issue :)  (The print statement is because it was a patch against py2.)  It really only needs a doc fix. The debug code is useful for debugging, and it is part of the public API of the module.  It would be nice to have it unit tested, but it isn't as big a deal as other code since it is unlikely to be used in production, only during development.

Finally, if the approach in this patch is accepted it can only go into 3.2, since it represents a significant behavior change.

I discussed this issue with Antoine Pitrou on #python-dev, and his opinion is that SMTPSocketConnectError doesn't add enough value to be worthwhile.  So he is in favor of making this a doc fix.

However, the suggestion also came up to have SMTPException subclass from IOError instead of Exception, since every place where an SMTPException is raised IO is involved.  This change would mean that code that didn't care what kind of IO error causes the connection to fail could trap just IOError, and code that did care could trap SMTPConnectError and IOError separately.

It is possible this would have backward compatibility issues, so the base class change is probably suitable only for 3.2, but a doc change clarifying that non-SMTP errors can be raised by connect should be backported.