NNTP constructor exception leaves socket for garbage collector

[vadmium]

BPO	22351
Nosy	@pitrou, @vstinner, @vadmium, @serhiy-storchaka
Files	issue22351.patch: patch for the issue issue22351_1.patch: updated patch for 22351 issue22351_2.patch issue22351_nntp_fail_3.patch issue22351_nntp_fail_4.patch issue22351_nntp_fail_5.patch issue22351_skip_MockSslTests.patch

^{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 = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2015-04-03.12:12:21.626>
created_at = <Date 2014-09-07.04:07:07.988>
labels = ['easy', 'library', 'performance']
title = 'NNTP constructor exception leaves socket for garbage collector'
updated_at = <Date 2015-04-03.12:12:21.625>
user = 'https://github.com/vadmium'

bugs.python.org fields:

activity = <Date 2015-04-03.12:12:21.625>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2015-04-03.12:12:21.626>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2014-09-07.04:07:07.988>
creator = 'martin.panter'
dependencies = []
files = ['36811', '36819', '36873', '38137', '38489', '38515', '38814']
hgrepos = []
issue_num = 22351
keywords = ['patch', 'easy']
message_count = 23.0
messages = ['226528', '228558', '228616', '229044', '235946', '235948', '236899', '238114', '238115', '238256', '238768', '238770', '239961', '239963', '239964', '239966', '239972', '239973', '239976', '239977', '239978', '239979', '239980']
nosy_count = 6.0
nosy_names = ['pitrou', 'vstinner', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'rishi.maker.forum']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue22351'
versions = ['Python 3.4', 'Python 3.5']

[vadmium]

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.

[rishimakerforum]

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.

[rishimakerforum]

patch updated to use just plain exception

[rishimakerforum]

patch updated based on comments.

[vadmium]

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.

[vadmium]

Posting a new patch which closes the file object, and also does the same for the SSL class.

[serhiy-storchaka]

Is it possible to make tests based on mocked server instead of real local server?

NNTP_SSL still is not tested.

[vadmium]

Posting issue22351_nntp_fail_4.patch; changes:

• Merge mixin and main TestCase class; mixins like this are sometimes useful to work around bpo-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.

[vadmium]

Oops, meant to link to bpo-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.

[vadmium]

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

[serhiy-storchaka]

Thanks Martin. The patch LGTM.