classification
Title: test_urllibnet is triggering a ResourceWarning
Type: Stage: resolved
Components: Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: brett.cannon, haypo, martin.panter, mcjeff, nadeem.vawda, orsenthil, python-dev
Priority: normal Keywords: patch

Created on 2011-03-15 22:57 by brett.cannon, last changed 2013-12-04 07:43 by martin.panter. This issue is now closed.

Files
File name Uploaded Description Edit
11562.patch mcjeff, 2011-03-17 18:30 review
unfakehttp.diff nadeem.vawda, 2011-03-19 17:00 Fix failures in test_urllib2_localnet, test_urllib2net and test_urllibnet. review
Messages (13)
msg131059 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2011-03-15 22:57
test.test_urllibnet.urlopenNetworkTests.test_getcode() is leaving a socket open. My guess is that the error condition being triggered is somehow leaving the socket open but I can't find where.
msg131224 - (view) Author: Jeff McNeil (mcjeff) * Date: 2011-03-17 05:48
So, I've been meaning to get more into contributing back to Python and I found this one somewhat interesting.

As it turns out, even the following simple script raises the same warning:


[jeff@martian cpython]$ ./python -c 'import urllib.request; urllib.request.urlretrieve("http://www.python.org")'
/home/jeff/cpython/Lib/socket.py:340: ResourceWarning: unclosed <socket.socket object, fd=3, family=2, type=1, proto=6>
  self._sock = None
[64388 refs]
[jeff@martian cpython]$ 

The close method of Socket.SocketIO simply sets the underlying socket object to None, which causes that warning.  Explicitly calling the close method on the underlying socket clears that up (and it's protected by that reference counter).

The _decref_socketios just drops the internal ref count and never actually closes -- it won't unless self.__closed is True. 

So, when self._sock is set to None, that error bubbles up. As SocketIO is the foundation used in socket.makefile, I think just adding that close call ought to be correct.

I can do the simple patch if you agree.
msg131225 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2011-03-17 05:58
Yes, go ahead with the patch.
msg131277 - (view) Author: Jeff McNeil (mcjeff) * Date: 2011-03-17 18:30
So, it turned out to be more complicated than that.  The HTTPConnection object returns an HTTPResponse, but never closes the underlying socket after calling makesock. 

Since persistent connections aren't supported, nothing actually closes  the socket itself, it's just set to None.  Explicitly calling a close turns out not to be correct either.

I went down the same path as AbstractHTTPHandler and added a Connection: close header.  That ensures that the remote host will close the underlying connection (more importantly, setting the HTTP Response object's will_close to True).  That ensures  HTTPConnection performs in a "fire and forget" mode, causing everything to close out as it should.

I contemplated changing urlretrieve to use build_opener as urlopen does, but I figure that would have been done by now if it was a trivial operation. I'd be happy to take a whack at it if it's just a matter of getting around to it.
msg131311 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2011-03-18 02:30
On Thu, Mar 17, 2011 at 06:30:51PM +0000, Jeff McNeil wrote:

> I went down the same path as AbstractHTTPHandler and added a Connection: close header.  

This is fine for the moment, tough I wish that the TODO pending in
urllib.request with HTTP1.1 persistent connection be removed soon and
will require changes it other places too.

> I contemplated changing urlretrieve to use build_opener as urlopen

There is bug opened for this. All that would be required is output
behavior of urlretrieve remain same, the implementation details (using
build_opener or using urlopen itself!) may not be relevant and I
think, it can be implemented using urlopen instead going through the
handlers and OpenerDirector, BTW, urlretrive is a convenience
function of some sort.
msg131312 - (view) Author: Jeff McNeil (mcjeff) * Date: 2011-03-18 02:42
Sounds good. I'll look at doing that, too.
msg131337 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2011-03-18 16:45
issue10883 is related; test_urllib2net also leaves sockets open in several places.
msg131402 - (view) Author: Roundup Robot (python-dev) Date: 2011-03-19 09:47
New changeset edc3d3b07435 by Senthil Kumaran in branch '3.2':
Closes issue11563 - test_urllibnet ResourceWarning. Patch by Jeff McNeil.
http://hg.python.org/cpython/rev/edc3d3b07435

New changeset dfceb98767c0 by Senthil Kumaran in branch 'default':
Closes issue11563 test_urllibnet is triggering a ResourceWarning. Patch by Jeff McNeil.
http://hg.python.org/cpython/rev/dfceb98767c0
msg131404 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-03-19 10:20
@Senthil Kumaran: Because your patch touchs not only the test, can you document your change in Misc/NEWS? Sending a new HTTP header should be documented.

Is there an issue to support persistent connections in AbstractHTTPHandler.do_open()?
msg131429 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2011-03-19 17:00
urlopen_HttpTests.test_willclose() fails to call unfakehttp(), which breaks subsequent runs of test_urllib2_localnet, test_urllib2net and test_urllibnet. Fix attached.
msg131458 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2011-03-19 23:02
Victor - Issue9740 and Issue3566 talks about the need to have persistent connection. 3.3 would be a good target to have this feature in.
Shall add the NEWS entry.

Nadeem Vawda - Thanks for your patch. I committed the fix as part of another bug (Issue3566). I shall mention the credits in an update to the log. thanks.
msg131466 - (view) Author: Roundup Robot (python-dev) Date: 2011-03-20 01:27
New changeset 53c8f2bd0316 by Senthil Kumaran in branch '3.2':
Add NEWS for  Issue #11563.
http://hg.python.org/cpython/rev/53c8f2bd0316
msg205204 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2013-12-04 07:43
I think the fix for this bug only works if it gets the server to respond with a “Connection: close” header itself. I opened Issue 19524 because I was seeing keep-alive responses using chunked encoding that still trigger a socket leak.
History
Date User Action Args
2013-12-04 07:43:46martin.pantersetnosy: + martin.panter
messages: + msg205204
2011-03-20 01:27:38python-devsetnosy: brett.cannon, orsenthil, haypo, nadeem.vawda, mcjeff, python-dev
messages: + msg131466
2011-03-19 23:02:36orsenthilsetnosy: brett.cannon, orsenthil, haypo, nadeem.vawda, mcjeff, python-dev
messages: + msg131458
2011-03-19 23:02:06orsenthilsetnosy: brett.cannon, orsenthil, haypo, nadeem.vawda, mcjeff, python-dev
messages: - msg131456
2011-03-19 23:01:25orsenthilsetnosy: brett.cannon, orsenthil, haypo, nadeem.vawda, mcjeff, python-dev
messages: + msg131456
2011-03-19 17:00:20nadeem.vawdasetfiles: + unfakehttp.diff
nosy: brett.cannon, orsenthil, haypo, nadeem.vawda, mcjeff, python-dev
messages: + msg131429
2011-03-19 10:20:27hayposetnosy: brett.cannon, orsenthil, haypo, nadeem.vawda, mcjeff, python-dev
messages: + msg131404
2011-03-19 09:47:39python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg131402

resolution: fixed
stage: needs patch -> resolved
2011-03-18 16:45:00nadeem.vawdasetnosy: + haypo, nadeem.vawda
messages: + msg131337
2011-03-18 02:42:19mcjeffsetnosy: brett.cannon, orsenthil, mcjeff
messages: + msg131312
versions: + Python 3.3
2011-03-18 02:30:11orsenthilsetnosy: brett.cannon, orsenthil, mcjeff
messages: + msg131311
2011-03-17 18:30:47mcjeffsetfiles: + 11562.patch

messages: + msg131277
keywords: + patch
nosy: brett.cannon, orsenthil, mcjeff
2011-03-17 05:58:18orsenthilsetnosy: + orsenthil
messages: + msg131225
2011-03-17 05:48:05mcjeffsetnosy: brett.cannon, mcjeff
messages: + msg131224
2011-03-17 04:18:28mcjeffsetnosy: + mcjeff
2011-03-15 22:57:35brett.cannoncreate