Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
stranger4good mannequin opened this issue Feb 14, 2008 · 22 comments
Closed

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

stranger4good mannequin opened this issue Feb 14, 2008 · 22 comments
Labels
easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@stranger4good
Copy link
Mannequin

stranger4good mannequin commented Feb 14, 2008

BPO 2118
Nosy @pitrou, @giampaolo, @tiran, @bitdancer, @asvetlov, @serhiy-storchaka
Files
  • issue_2118_fixed.patch: Fixes the issue and changes the test file
  • issue_2118_fix_for_py3.patch
  • issue2118.diff
  • issue2118-doc-patch-3.3.diff
  • issue2118-OSError.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2013-04-13.18:54:07.038>
    created_at = <Date 2008-02-14.22:23:12.234>
    labels = ['easy', 'type-bug', 'library']
    title = 'smtplib.SMTP() raises socket.error rather than SMTPConnectError'
    updated_at = <Date 2013-04-14.10:46:52.717>
    user = 'https://bugs.python.org/stranger4good'

    bugs.python.org fields:

    activity = <Date 2013-04-14.10:46:52.717>
    actor = 'python-dev'
    assignee = 'none'
    closed = True
    closed_date = <Date 2013-04-13.18:54:07.038>
    closer = 'r.david.murray'
    components = ['Library (Lib)']
    creation = <Date 2008-02-14.22:23:12.234>
    creator = 'stranger4good'
    dependencies = []
    files = ['9509', '18301', '29799', '29805', '29830']
    hgrepos = []
    issue_num = 2118
    keywords = ['patch', 'easy']
    message_count = 22.0
    messages = ['62416', '62423', '62447', '62451', '62747', '62785', '68515', '110631', '112261', '112264', '116266', '186744', '186748', '186758', '186792', '186795', '186797', '186810', '186812', '186841', '186850', '186908']
    nosy_count = 15.0
    nosy_names = ['gjb1002', 'pitrou', 'giampaolo.rodola', 'christian.heimes', 'stranger4good', 'zanella', 'werneck', 'rodolpho', 'r.david.murray', 'asvetlov', 'BreamoreBoy', 'gregmalcolm', 'python-dev', 'serhiy.storchaka', 'n']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue2118'
    versions = ['Python 2.7', 'Python 3.3', 'Python 3.4']

    @stranger4good
    Copy link
    Mannequin Author

    stranger4good mannequin commented Feb 14, 2008

    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')

    @stranger4good stranger4good mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Feb 14, 2008
    @tiran
    Copy link
    Member

    tiran commented Feb 15, 2008

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

    @stranger4good
    Copy link
    Mannequin Author

    stranger4good mannequin commented Feb 16, 2008

    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

    @tiran
    Copy link
    Member

    tiran commented Feb 16, 2008

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

    @tiran tiran added the easy label Feb 16, 2008
    @werneck
    Copy link
    Mannequin

    werneck mannequin commented Feb 23, 2008

    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.

    @werneck
    Copy link
    Mannequin

    werneck mannequin commented Feb 23, 2008

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

    @rodolpho
    Copy link
    Mannequin

    rodolpho mannequin commented Jun 21, 2008

    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)

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Jul 18, 2010

    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?

    @gregmalcolm
    Copy link
    Mannequin

    gregmalcolm mannequin commented Aug 1, 2010

    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.

    @bitdancer
    Copy link
    Member

    bitdancer commented Aug 1, 2010

    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.

    @bitdancer
    Copy link
    Member

    bitdancer commented Sep 13, 2010

    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.

    @n
    Copy link
    Mannequin

    n mannequin commented Apr 13, 2013

    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

    @bitdancer
    Copy link
    Member

    bitdancer commented Apr 13, 2013

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

    @n
    Copy link
    Mannequin

    n mannequin commented Apr 13, 2013

    ...and the 3.3 doc patch.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 13, 2013

    New changeset 6ca27b5de309 by R David Murray in branch '2.7':
    bpo-2118: clarify smtplib exception documentation.
    http://hg.python.org/cpython/rev/6ca27b5de309

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

    New changeset 75e7bd46fcd7 by R David Murray in branch 'default':
    Merge bpo-2118: clarify smtplib exception documentation.
    http://hg.python.org/cpython/rev/75e7bd46fcd7

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 13, 2013

    New changeset 1158c38bf2dc by R David Murray in branch 'default':
    bpo-2118: Make SMTPException a subclass of IOError.
    http://hg.python.org/cpython/rev/1158c38bf2dc

    @bitdancer
    Copy link
    Member

    bitdancer commented Apr 13, 2013

    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.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Apr 13, 2013

    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.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Apr 13, 2013

    socket.error is another alias of OSError.

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Apr 13, 2013

    Agree with Serhiy. Better to direcly use OSError than IOError alias.

    @n
    Copy link
    Mannequin

    n mannequin commented Apr 13, 2013

    s/IOError/OSError/

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 14, 2013

    New changeset adc72ff451dc by R David Murray in branch 'default':
    bpo-2118: IOError is deprecated, use OSError.
    http://hg.python.org/cpython/rev/adc72ff451dc

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants