[issue22351] NNTP constructor exception leaves socket for garbage collector

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Now you have used time machine in reverse mode.

--
resolution:  -> fixed
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[STINNER Victor]

STINNER Victor added the comment:

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

--
resolution: fixed -> 
status: closed -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[STINNER Victor]

STINNER Victor added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Roundup Robot]

Roundup Robot added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Agree, factory method can be staticmethod.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Martin Panter]

Martin Panter added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Martin Panter]

Martin Panter added the comment:

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!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

See the patch.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[STINNER Victor]

STINNER Victor added the comment:

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

I don't know how to do that.

--
nosy: +haypo

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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

--
Added file: http://bugs.python.org/file38814/issue22351_skip_MockSslTests.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Roundup Robot]

Roundup Robot added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
resolution:  -> fixed
stage: commit review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Roundup Robot]

Roundup Robot added the comment:

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

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Thanks Martin. The patch LGTM.

--
stage: test needed -> commit review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Martin Panter]

Martin Panter added the comment:

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

--
Added file: http://bugs.python.org/file38515/issue22351_nntp_fail_5.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Martin Panter]

Martin Panter added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Martin Panter]

Martin Panter added the comment:

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.

--
Added file: http://bugs.python.org/file38489/issue22351_nntp_fail_4.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
stage: patch review -> test needed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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

NNTP_SSL still is not tested.

--
versions: +Python 3.5

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
assignee:  -> serhiy.storchaka
nosy: +serhiy.storchaka
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Martin Panter]

Martin Panter added the comment:

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

--
Added file: http://bugs.python.org/file38137/issue22351_nntp_fail_3.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Martin Panter]

Martin Panter added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Rishi]

Rishi added the comment:

patch updated based on comments.

--
Added file: http://bugs.python.org/file36873/issue22351_2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Rishi]

Rishi added the comment:

patch updated to use just plain exception

--
Added file: http://bugs.python.org/file36819/issue22351_1.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Rishi]

Rishi added the comment:

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.

--
keywords: +patch
nosy: +rishi.maker.forum
Added file: http://bugs.python.org/file36811/issue22351.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Benjamin Peterson]

Changes by Benjamin Peterson :


--
keywords: +easy

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Terry J. Reedy]

Changes by Terry J. Reedy :


--
nosy: +pitrou

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22351] NNTP constructor exception leaves socket for garbage collector

[Martin Panter]

New submission from Martin Panter:

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

--
components: Library (Lib)
messages: 226528
nosy: vadmium
priority: normal
severity: normal
status: open
title: NNTP constructor exception leaves socket for garbage collector
type: resource usage
versions: Python 3.4

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com