classification
Title: smtplib.SMTP() raises socket.error rather than SMTPConnectError
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.4, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: BreamoreBoy, asvetlov, christian.heimes, giampaolo.rodola, gjb1002, gregmalcolm, n, pitrou, python-dev, r.david.murray, rodolpho, serhiy.storchaka, stranger4good, werneck, zanella
Priority: normal Keywords: easy, patch

Created on 2008-02-14 22:23 by stranger4good, last changed 2013-04-14 10:46 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
issue_2118_fixed.patch werneck, 2008-02-23 17:50 Fixes the issue and changes the test file
issue_2118_fix_for_py3.patch gregmalcolm, 2010-08-01 03:32
issue2118.diff n, 2013-04-13 16:12 review
issue2118-doc-patch-3.3.diff n, 2013-04-13 16:47 review
issue2118-OSError.diff n, 2013-04-13 21:13 review
Messages (22)
msg62416 - (view) Author: (stranger4good) Date: 2008-02-14 22:23
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')
msg62423 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-02-15 08:16
Why is this a bug? Do the docs promise that smtplib only raises
SMTPConnectError?
msg62447 - (view) Author: (stranger4good) Date: 2008-02-16 01:38
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
msg62451 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-02-16 07:39
I see! You are right. Either the docs or the code need an update
msg62747 - (view) Author: Pedro Werneck (werneck) Date: 2008-02-23 15:23
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.
msg62785 - (view) Author: Pedro Werneck (werneck) Date: 2008-02-23 17:50
Previous patch didn't passed the tests right. This patch fixes both the
code, unindenting port number conversion to integer and the test.
msg68515 - (view) Author: Rodolpho Eckhardt (rodolpho) Date: 2008-06-21 16:50
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)
msg110631 - (view) Author: Mark Lawrence (BreamoreBoy) Date: 2010-07-18 11:09
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?
msg112261 - (view) Author: Greg Malcolm (gregmalcolm) Date: 2010-08-01 03:32
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.
msg112264 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-08-01 04:58
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.
msg116266 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-09-13 01:19
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.
msg186744 - (view) Author: Ned Jackson Lovely (n) * Date: 2013-04-13 16:12
Attaching a patch to make SMTPException an IOError, with corresponding update to docs to point out that __init__ on the SMTP object will raise IOErrors in general, and some SMTPExceptions in particular.

Boston Python Sprint Apr 2013
msg186748 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-04-13 16:26
This looks good for 3.4.  Ned, would you also be willing to prepare a doc patch for 3.3 that mentions that IOError may be raised?  (I think the 3.3 patch will also apply to 2.7.)
msg186758 - (view) Author: Ned Jackson Lovely (n) * Date: 2013-04-13 16:47
...and the 3.3 doc patch.
msg186792 - (view) Author: Roundup Robot (python-dev) Date: 2013-04-13 18:43
New changeset 6ca27b5de309 by R David Murray in branch '2.7':
#2118: clarify smtplib exception documentation.
http://hg.python.org/cpython/rev/6ca27b5de309

New changeset 36d07a877f33 by R David Murray in branch '3.3':
#2118: clarify smtplib exception documentation.
http://hg.python.org/cpython/rev/36d07a877f33

New changeset 75e7bd46fcd7 by R David Murray in branch 'default':
Merge #2118: clarify smtplib exception documentation.
http://hg.python.org/cpython/rev/75e7bd46fcd7
msg186795 - (view) Author: Roundup Robot (python-dev) Date: 2013-04-13 18:50
New changeset 1158c38bf2dc by R David Murray in branch 'default':
#2118: Make SMTPException a subclass of IOError.
http://hg.python.org/cpython/rev/1158c38bf2dc
msg186797 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-04-13 18:54
Thanks, Ned.  After taking a look at the library documentation, I decided to change the docstring in a somewhat different way.  In general in our docs we do not document all the possible exceptions a library can raise, but just the library-specific exceptions and the interesting special cases.  So what I did was to remove the language that implied that *only* SMTPExceptions were raised.  And then in the 3.4 docs noted that it is a subclass of IOError.
msg186810 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-04-13 19:50
Since 3.3 IOError is an alias of OSError. We get rid of reference of IOError and other OSError aliases in the documentation and code, replacing them with OSError.
msg186812 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-04-13 19:54
socket.error is another alias of OSError.
msg186841 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2013-04-13 20:52
Agree with Serhiy. Better to direcly use OSError than IOError alias.
msg186850 - (view) Author: Ned Jackson Lovely (n) * Date: 2013-04-13 21:13
s/IOError/OSError/
msg186908 - (view) Author: Roundup Robot (python-dev) Date: 2013-04-14 10:46
New changeset adc72ff451dc by R David Murray in branch 'default':
#2118: IOError is deprecated, use OSError.
http://hg.python.org/cpython/rev/adc72ff451dc
History
Date User Action Args
2013-04-14 10:46:52python-devsetmessages: + msg186908
2013-04-13 21:13:11nsetfiles: + issue2118-OSError.diff

messages: + msg186850
2013-04-13 20:52:52asvetlovsetnosy: + asvetlov
messages: + msg186841
2013-04-13 19:54:16serhiy.storchakasetmessages: + msg186812
2013-04-13 19:50:25serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg186810
2013-04-13 18:54:07r.david.murraysetstatus: open -> closed
resolution: fixed
messages: + msg186797

stage: needs patch -> resolved
2013-04-13 18:50:09python-devsetmessages: + msg186795
2013-04-13 18:43:10python-devsetnosy: + python-dev
messages: + msg186792
2013-04-13 16:47:04nsetfiles: + issue2118-doc-patch-3.3.diff

messages: + msg186758
2013-04-13 16:26:23r.david.murraysetmessages: + msg186748
versions: + Python 2.7, Python 3.3, Python 3.4, - Python 3.2
2013-04-13 16:12:20nsetfiles: + issue2118.diff
nosy: + n
messages: + msg186744

2010-09-13 01:19:34r.david.murraysetnosy: + pitrou, giampaolo.rodola
messages: + msg116266

resolution: accepted -> (no value)
stage: patch review -> needs patch
2010-08-01 04:58:39r.david.murraysetnosy: + r.david.murray

messages: + msg112264
versions: + Python 3.2, - Python 2.6, Python 3.1
2010-08-01 03:32:47gregmalcolmsetfiles: + issue_2118_fix_for_py3.patch
nosy: + gregmalcolm
messages: + msg112261

2010-07-18 11:09:45BreamoreBoysetnosy: + BreamoreBoy
messages: + msg110631
2010-07-05 14:01:32gjb1002setnosy: + gjb1002
2009-05-12 12:43:50ajaksu2setkeywords: + patch
stage: patch review
versions: + Python 3.1, - Python 2.5, Python 3.0
2008-06-21 16:51:09rodolphosetnosy: + rodolpho
messages: + msg68515
versions: + Python 2.6, Python 3.0
2008-05-10 13:07:20wernecksetfiles: - issue_2118_smtplib.patch
2008-02-23 17:50:21wernecksetfiles: + issue_2118_fixed.patch
messages: + msg62785
2008-02-23 15:23:14wernecksetfiles: + issue_2118_smtplib.patch
nosy: + werneck
messages: + msg62747
2008-02-23 14:31:22zanellasetnosy: + zanella
2008-02-16 07:39:59christian.heimessetkeywords: + easy
resolution: accepted
messages: + msg62451
2008-02-16 01:38:16stranger4goodsetmessages: + msg62447
2008-02-15 08:16:32christian.heimessetpriority: normal
nosy: + christian.heimes
messages: + msg62423
2008-02-14 22:23:12stranger4goodcreate