smtplib leaves open sockets around if SMTPConnectError is raised in __init__

[jhillacre]

BPO	30394
Nosy	@bitdancer, @zhangyangyu, @jhillacre
PRs	bpo-30394: smtplib leaves open sockets around if SMTPConnectError is raised in __init__ #1700 [2.7] bpo-30394: Fix a socket leak in smtplib.SMTP.__init__() #1788 [3.5] bpo-30394: Fix a socket leak in smtplib.SMTP.__init__() #1789 [3.6] bpo-30394: Fix a socket leak in smtplib.SMTP.__init__() #1790
Files	smtplib.patch test_server.py test_client.py

^{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 2017-05-24.19:04:12.954>
created_at = <Date 2017-05-17.23:25:01.619>
labels = ['3.7', 'type-bug', 'library']
title = 'smtplib leaves open sockets around if SMTPConnectError is raised in __init__'
updated_at = <Date 2017-05-24.19:04:12.953>
user = 'https://github.com/jhillacre'

bugs.python.org fields:

activity = <Date 2017-05-24.19:04:12.953>
actor = 'xiang.zhang'
assignee = 'none'
closed = True
closed_date = <Date 2017-05-24.19:04:12.954>
closer = 'xiang.zhang'
components = ['Library (Lib)']
creation = <Date 2017-05-17.23:25:01.619>
creator = 'jhillacre'
dependencies = []
files = ['46873', '46874', '46875']
hgrepos = []
issue_num = 30394
keywords = ['patch']
message_count = 10.0
messages = ['293905', '294103', '294112', '294191', '294263', '294284', '294322', '294372', '294376', '294377']
nosy_count = 3.0
nosy_names = ['r.david.murray', 'xiang.zhang', 'jhillacre']
pr_nums = ['1700', '1788', '1789', '1790']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue30394'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6', 'Python 3.7']

[jhillacre]

I am encountering a ResourceWarning about an unclosed socket when getting a non 220 response during connect() in __init__() of smtplib.SMTP. Attached are a client script causing this warning for me, a server script to cause the client to the warning and a patch that fixes the warning for me. My python version is Python 3.6.1 (default, Apr 7 2017, 09:32:32) [GCC 4.8.5 20150623 (Red Hat 4.8.5-11)] on linux. I had found previous related issue with similar symptom and remedy in issue bpo-21641.

$ python3.6 test_server.py
connected by ('127.0.0.1', 53630)
$ 

$ python3.6 test_client.py
Traceback (most recent call last):
  File "test_client.py", line 2, in <module>
    smtp = smtplib.SMTP('127.0.0.1', 8025)
  File "/usr/lib64/python3.6/smtplib.py", line 253, in __init__
    raise SMTPConnectError(code, msg)
smtplib.SMTPConnectError: (554, b'Nope.')
/usr/lib64/python3.6/socket.py:657: ResourceWarning: unclosed <socket.socket fd=4, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=6, laddr=('127.0.0.1', 53630), raddr=('127.0.0.1', 8025)>
$ 

RFC 2821 states that servers responding with non 220 greetings must not close the connection. It is this behaviour that test_server.py is using to trigger the warning in test_client.py.

RFC 2821 Section 3.1 Paragraph 3
https://tools.ietf.org/html/rfc2821#section-3.1
'... a 554 response MAY be given in the initial connection opening message
instead of the 220. A server taking this approach MUST still wait
for the client to send a QUIT (see section 4.1.1.10) before closing
the connection and SHOULD respond to any intervening commands with
"503 bad sequence of commands".'

The ResourceWarning is no longer caused for me after applying this change.

[zhangyangyu]

Joel, our patch system has moved to GitHub. Mind to turn your patch into a PR?

[jhillacre]

Joel, our patch system has moved to GitHub. Mind to turn your patch into a PR?

I have opened a PR now.

I took a look at changing my example into a test. I am not sure how to test for a lack of warning. Closest test I found was BadHELOServerTests in test_smtplib.py, but using mock_socket would not cause the warning from socket.py.

[bitdancer]

It would be a white-box test, which I don't like, but it might be worth it to write a test that would check that <smtpobj>.sock is None, indicating that close was called. You really can't check for no warning because when the warning gets generated is effectively asynchronous.

[jhillacre]

r.david.murray,

How would a test would have a reference to <smtpobj> after an exception in the smtplib.SMTP.__init__()?

[bitdancer]

Duh. (Smacks self on forehead).

Nevermind.

I'll approve the patch as is, since I can't see any good way to test it.

(I suppose that we could factor the __init__ method contents out into something we could test, but I'm not going to push for that kind of refactoring for this simple change. It probably comes down to connect being called in init being a design bug :)

[zhangyangyu]

New changeset 9e98cd0 by Xiang Zhang (Joel Hillacre) in branch 'master':
bpo-30394: Fix a socket leak in smtplib.SMTP.__init__() (bpo-1700)
9e98cd0

[zhangyangyu]

New changeset 779e7c9 by Xiang Zhang (Stéphane Wirtel) in branch '2.7':
bpo-30394: Fix a socket leak in smtplib.SMTP.__init__() (bpo-1700) (bpo-1788)
779e7c9

[zhangyangyu]

New changeset ebbefae by Xiang Zhang (Stéphane Wirtel) in branch '3.5':
bpo-30394: Fix a socket leak in smtplib.SMTP.__init__() (bpo-1700) (bpo-1789)
ebbefae

[zhangyangyu]

New changeset c3454f0 by Xiang Zhang (Stéphane Wirtel) in branch '3.6':
bpo-30394: Fix a socket leak in smtplib.SMTP.__init__() (bpo-1700) (bpo-1790)
c3454f0