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.

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.

patch updated to use just plain exception

patch updated based on comments.

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.

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

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

NNTP_SSL still is not tested.

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.

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.

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

Thanks Martin. The patch LGTM.

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

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

Good catch. But may be better to report MockSslTests as skipped instead of silently omit them.

> Good catch. But may be better to report MockSslTests as skipped instead of silently omit them.

I don't know how to do that.

See the patch.

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!

Sorry, that self parameter is okay. I was just confused because it now means something different :S

Agree, factory method can be staticmethod.

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

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

Except of the self parameter/@staticmethod and the unrelated SAX changes, issue22351_skip_MockSslTests.patch looks good to me.

Now you have used time machine in reverse mode.