classification
Title: smtplib leaves open sockets around if SMTPConnectError is raised in __init__
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.7, Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: jhillacre, r.david.murray, xiang.zhang
Priority: normal Keywords: patch

Created on 2017-05-17 23:25 by jhillacre, last changed 2017-05-24 19:04 by xiang.zhang. This issue is now closed.

Files
File name Uploaded Description Edit
smtplib.patch jhillacre, 2017-05-17 23:25
test_server.py jhillacre, 2017-05-17 23:25
test_client.py jhillacre, 2017-05-17 23:25
Pull Requests
URL Status Linked Edit
PR 1700 merged jhillacre, 2017-05-21 20:32
PR 1788 merged matrixise, 2017-05-24 17:13
PR 1789 merged matrixise, 2017-05-24 17:13
PR 1790 merged matrixise, 2017-05-24 17:13
Messages (10)
msg293905 - (view) Author: Joel Hillacre (jhillacre) * Date: 2017-05-17 23:25
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 #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.
msg294103 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2017-05-21 14:34
Joel, our patch system has moved to GitHub. Mind to turn your patch into a PR?
msg294112 - (view) Author: Joel Hillacre (jhillacre) * Date: 2017-05-21 20:41
> 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.
msg294191 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2017-05-22 21:47
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.
msg294263 - (view) Author: Joel Hillacre (jhillacre) * Date: 2017-05-23 16:47
r.david.murray,

How would a test would have a reference to <smtpobj> after an exception in the smtplib.SMTP.__init__()?
msg294284 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2017-05-23 21:11
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 :)
msg294322 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2017-05-24 05:14
New changeset 9e98cd0383d9e7f06c0537038a32459bf5efa97a by Xiang Zhang (Joel Hillacre) in branch 'master':
bpo-30394: Fix a socket leak in smtplib.SMTP.__init__() (#1700)
https://github.com/python/cpython/commit/9e98cd0383d9e7f06c0537038a32459bf5efa97a
msg294372 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2017-05-24 18:24
New changeset 779e7c933e777270897b1e35fa9e5b12eee12af9 by Xiang Zhang (Stéphane Wirtel) in branch '2.7':
bpo-30394: Fix a socket leak in smtplib.SMTP.__init__() (#1700) (#1788)
https://github.com/python/cpython/commit/779e7c933e777270897b1e35fa9e5b12eee12af9
msg294376 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2017-05-24 18:49
New changeset ebbefae14039aa86d4c8a7cfab8f2b5a3ef0d241 by Xiang Zhang (Stéphane Wirtel) in branch '3.5':
bpo-30394: Fix a socket leak in smtplib.SMTP.__init__() (#1700) (#1789)
https://github.com/python/cpython/commit/ebbefae14039aa86d4c8a7cfab8f2b5a3ef0d241
msg294377 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2017-05-24 18:59
New changeset c3454f0e79b35fb81b0426cfac4b801d4495b8ea by Xiang Zhang (Stéphane Wirtel) in branch '3.6':
bpo-30394: Fix a socket leak in smtplib.SMTP.__init__() (#1700) (#1790)
https://github.com/python/cpython/commit/c3454f0e79b35fb81b0426cfac4b801d4495b8ea
History
Date User Action Args
2017-05-24 19:04:12xiang.zhangsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2017-05-24 18:59:08xiang.zhangsetmessages: + msg294377
2017-05-24 18:49:26xiang.zhangsetmessages: + msg294376
2017-05-24 18:24:29xiang.zhangsetmessages: + msg294372
2017-05-24 17:13:57matrixisesetpull_requests: + pull_request1872
2017-05-24 17:13:45matrixisesetpull_requests: + pull_request1871
2017-05-24 17:13:15matrixisesetpull_requests: + pull_request1870
2017-05-24 05:14:53xiang.zhangsetmessages: + msg294322
2017-05-23 21:11:17r.david.murraysetmessages: + msg294284
2017-05-23 16:47:27jhillacresetmessages: + msg294263
2017-05-22 21:47:39r.david.murraysetnosy: + r.david.murray
messages: + msg294191
2017-05-21 20:41:09jhillacresetmessages: + msg294112
2017-05-21 20:32:56jhillacresetpull_requests: + pull_request1796
2017-05-21 14:34:42xiang.zhangsetversions: + Python 2.7, Python 3.5, Python 3.7
nosy: + xiang.zhang

messages: + msg294103

type: enhancement -> behavior
stage: patch review
2017-05-17 23:25:17jhillacresetfiles: + test_client.py
2017-05-17 23:25:12jhillacresetfiles: + test_server.py
2017-05-17 23:25:01jhillacrecreate