classification
Title: NNTP constructor exception leaves socket for garbage collector
Type: resource usage Stage: resolved
Components: Library (Lib) Versions: Python 3.5, Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: haypo, martin.panter, pitrou, python-dev, rishi.maker.forum, serhiy.storchaka
Priority: normal Keywords: easy, patch

Created on 2014-09-07 04:07 by martin.panter, last changed 2015-04-03 12:12 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
issue22351.patch rishi.maker.forum, 2014-10-05 10:52 patch for the issue review
issue22351_1.patch rishi.maker.forum, 2014-10-05 19:18 updated patch for 22351 review
issue22351_2.patch rishi.maker.forum, 2014-10-10 23:24 review
issue22351_nntp_fail_3.patch martin.panter, 2015-02-14 03:57 review
issue22351_nntp_fail_4.patch martin.panter, 2015-03-15 01:58 review
issue22351_nntp_fail_5.patch martin.panter, 2015-03-17 02:42 review
issue22351_skip_MockSslTests.patch serhiy.storchaka, 2015-04-03 09:26 review
Messages (23)
msg226528 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2014-09-07 04:07
If the nntplib.NNTP constructor fails, it often leaves the connection and socket open until the garbage collector cleans them up and emits a ResourceWarning:

>>> try:
...     NNTP("localhost")
... except Exception as err:
...     print(repr(err))
... 
NNTPTemporaryError('400 Service temporarily unavailable',)
>>> gc.collect()
__main__:1: ResourceWarning: unclosed <socket.socket fd=3, family=AddressFamily.AF_INET, type=SocketType.SOCK_STREAM, proto=6, laddr=('127.0.0.1', 54819), raddr=('127.0.0.1', 119)>
12

This happens both for error responses that are expected by the protocol, e.g. service unavailable as above, authentication errors. It also happens for other exceptions such as EOFError if the connection is closed with no response.
msg228558 - (view) Author: Rishi (rishi.maker.forum) * Date: 2014-10-05 10:52
Here is my attempt to fix this issue. This is my first patch ever :).
IMO checking socket leaks in the constructor requires an actual server, so I create an actual localhost dummy server and test some error conditions that are encountered by the constructor.
msg228616 - (view) Author: Rishi (rishi.maker.forum) * Date: 2014-10-05 19:18
patch updated to use just plain exception
msg229044 - (view) Author: Rishi (rishi.maker.forum) * Date: 2014-10-10 23:24
patch updated based on comments.
msg235946 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-02-14 02:12
I believe this still leaves the socket open via the “file” object. Just that there is no ResourceWarning generated due to the way IOBase.__del__() works.
msg235948 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-02-14 03:57
Posting a new patch which closes the file object, and also does the same for the SSL class.
msg236899 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-28 17:38
Is it possible to make tests based on mocked server instead of real local server?

NNTP_SSL still is not tested.
msg238114 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-03-15 01:58
Posting issue22351_nntp_fail_4.patch; changes:

* Merge mixin and main TestCase class; mixins like this are sometimes useful to work around Issue 22351, but not necessary in this case, because there is only one TestCase class involved.
* Turn the local server in the background thread into mock socket object, with makefile() returning a duplex stream object, reusing the existing _NNTPServerIO and NNTPv1Handler classes.
* Test socket closure by checking mock socket and stream objects directly, rather than relying on a lack of ResourceWarning.
msg238115 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-03-15 02:06
Oops, meant to link to Issue 14534 about the test case mixing hack.

BTW this new patch also tests NNTP_SSL, by bypassing the encryption step and repeating the non-SSL tests.
msg238256 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-03-17 02:42
Thanks for reviewing, Serhiy. I am posting a new patch addressing the comments:

* Removed underscore from method name
* Made separate MockSslTests subclass, using a hacked ssl_context parameter to bypass the SSL module
* Separated asserts for closed socket and file objects

I have left the tests patching the “nntplib” module and inserting a mock “socket” module. Serhiy suggested patching the “socket” module directly, but that seems to be asking for trouble. Alternatives I can think of are:

* Go back to Rishi’s original code that uses a real socket in a background thread.
* Do the patching in a subprocess. But this is awkward if you want to reuse the existing NNTPv1Handler class in the subprocess. Suggestions or patches welcome :)
* Refactor the “nntplib” code specially to make it easier to test without a real socket and without patching
msg238768 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-21 07:37
Thanks Martin. The patch LGTM.
msg238770 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-21 07:42
New changeset e8878579eb68 by Serhiy Storchaka in branch '3.4':
Issue #22351: The nntplib.NNTP constructor no longer leaves the connection
https://hg.python.org/cpython/rev/e8878579eb68

New changeset 6622f68b064b by Serhiy Storchaka in branch 'default':
Issue #22351: The nntplib.NNTP constructor no longer leaves the connection
https://hg.python.org/cpython/rev/6622f68b064b
msg239961 - (view) Author: Roundup Robot (python-dev) Date: 2015-04-03 09:09
New changeset c3e7a670dda2 by Victor Stinner in branch '3.4':
Issue #22351: Fix test_nntplib if the ssl module is missing
https://hg.python.org/cpython/rev/c3e7a670dda2
msg239963 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-04-03 09:26
Good catch. But may be better to report MockSslTests as skipped instead of silently omit them.
msg239964 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-04-03 09:27
> Good catch. But may be better to report MockSslTests as skipped instead of silently omit them.

I don't know how to do that.
msg239966 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-04-03 10:47
See the patch.
msg239972 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-04-03 11:46
Serhiy’s patch looks like it should do the trick. Just get rid of the “self” parameter to make it clearer, and watch out for the stray XML changes!
msg239973 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-04-03 11:48
Sorry, that self parameter is okay. I was just confused because it now means something different :S
msg239976 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-04-03 12:03
Agree, factory method can be staticmethod.
msg239977 - (view) Author: Roundup Robot (python-dev) Date: 2015-04-03 12:03
New changeset 7a91363f31e1 by Serhiy Storchaka in branch '3.4':
Issue #22351. MockSslTests tests in test_nntplib now are reported if skipped.
https://hg.python.org/cpython/rev/7a91363f31e1

New changeset c935c1e1d001 by Serhiy Storchaka in branch 'default':
Issue #22351. MockSslTests tests in test_nntplib now are reported if skipped.
https://hg.python.org/cpython/rev/c935c1e1d001
msg239978 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-04-03 12:05
> Serhiy’s patch looks like it should do the trick. Just get rid of the “self” parameter to make it clearer,

+class MockSslTests(MockSocketTests):
+    def nntp_class(self, *pos, **kw):

Hum, you should use the @staticmethod decorator here.
msg239979 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-04-03 12:06
Except of the self parameter/@staticmethod and the unrelated SAX changes, issue22351_skip_MockSslTests.patch looks good to me.
msg239980 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-04-03 12:12
Now you have used time machine in reverse mode.
History
Date User Action Args
2015-04-03 12:12:21serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg239980
2015-04-03 12:06:50hayposetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg239979
2015-04-03 12:05:36hayposetmessages: + msg239978
2015-04-03 12:03:57python-devsetmessages: + msg239977
2015-04-03 12:03:57serhiy.storchakasetmessages: + msg239976
2015-04-03 11:48:11martin.pantersetmessages: + msg239973
2015-04-03 11:46:55martin.pantersetmessages: + msg239972
2015-04-03 10:47:04serhiy.storchakasetmessages: + msg239966
2015-04-03 09:27:56hayposetnosy: + haypo
messages: + msg239964
2015-04-03 09:26:48serhiy.storchakasetfiles: + issue22351_skip_MockSslTests.patch

messages: + msg239963
2015-04-03 09:09:45python-devsetmessages: + msg239961
2015-03-21 07:46:00serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: commit review -> resolved
2015-03-21 07:42:05python-devsetnosy: + python-dev
messages: + msg238770
2015-03-21 07:37:09serhiy.storchakasetmessages: + msg238768
stage: test needed -> commit review
2015-03-17 02:42:19martin.pantersetfiles: + issue22351_nntp_fail_5.patch

messages: + msg238256
2015-03-15 02:06:57martin.pantersetmessages: + msg238115
2015-03-15 01:58:45martin.pantersetfiles: + issue22351_nntp_fail_4.patch

messages: + msg238114
2015-03-04 10:32:13serhiy.storchakasetstage: patch review -> test needed
2015-02-28 17:38:07serhiy.storchakasetmessages: + msg236899
versions: + Python 3.5
2015-02-28 16:22:30serhiy.storchakasetassignee: serhiy.storchaka

nosy: + serhiy.storchaka
stage: patch review
2015-02-14 03:57:43martin.pantersetfiles: + issue22351_nntp_fail_3.patch

messages: + msg235948
2015-02-14 02:12:59martin.pantersetmessages: + msg235946
2014-10-10 23:24:05rishi.maker.forumsetfiles: + issue22351_2.patch

messages: + msg229044
2014-10-05 19:18:10rishi.maker.forumsetfiles: + issue22351_1.patch

messages: + msg228616
2014-10-05 10:52:57rishi.maker.forumsetfiles: + issue22351.patch

nosy: + rishi.maker.forum
messages: + msg228558

keywords: + patch
2014-09-13 01:50:48benjamin.petersonsetkeywords: + easy
2014-09-12 20:11:50terry.reedysetnosy: + pitrou
2014-09-07 04:07:07martin.pantercreate