homepage

test_urllibnet.py and test_urllibnet2.py emit ResourceWarning:
==============
$ ./python Lib/test/test_urllibnet.py 
testURLread (__main__.URLTimeoutTest) ... ok
test_bad_address (__main__.urlopenNetworkTests) ... ok
test_basic (__main__.urlopenNetworkTests) ... ok
test_fileno (__main__.urlopenNetworkTests) ... ok
test_getcode (__main__.urlopenNetworkTests) ... /home/haypo/prog/GIT/py3k/Lib/socket.py:333: ResourceWarning: unclosed <socket.socket object, fd=3, family=2, type=1, proto=6>
 self._sock = None
ok
test_geturl (__main__.urlopenNetworkTests) ... ok
test_info (__main__.urlopenNetworkTests) ... ok
test_readlines (__main__.urlopenNetworkTests) ... ok
test_basic (__main__.urlretrieveNetworkTests) ... /home/haypo/prog/GIT/py3k/Lib/socket.py:333: ResourceWarning: unclosed <socket.socket object, fd=3, family=2, type=2049, proto=6>
 self._sock = None
ok
test_data_header (__main__.urlretrieveNetworkTests) ... /home/haypo/prog/GIT/py3k/Lib/socket.py:333: ResourceWarning: unclosed <socket.socket object, fd=3, family=2, type=2049, proto=6>
 self._sock = None
ok
test_header (__main__.urlretrieveNetworkTests) ... /home/haypo/prog/GIT/py3k/Lib/socket.py:333: ResourceWarning: unclosed <socket.socket object, fd=3, family=2, type=2049, proto=6>
 self._sock = None
ok
test_specified_path (__main__.urlretrieveNetworkTests) ... /home/haypo/prog/GIT/py3k/Lib/socket.py:333: ResourceWarning: unclosed <socket.socket object, fd=3, family=2, type=2049, proto=6>
 self._sock = None
ok
----------------------------------------------------------------------
Ran 12 tests in 17.473s
==============
It looks like these warning are real bugs: the socket is not closed explicitly by urllib.
Nadeem Vawda suggests a first fix:
 diff --git a/Lib/urllib/request.py b/Lib/urllib/request.py
 --- a/Lib/urllib/request.py
 +++ b/Lib/urllib/request.py
 @@ -2151,7 +2151,9 @@
 conn = self.ftp.ntransfercmd(cmd)
 self.busy = 1
 # Pass back both a suitably decorated object and a retrieval length
 - return (addclosehook(conn[0].makefile('rb'), self.endtransfer), conn[1])
 + fp = addclosehook(conn[0].makefile('rb'), self.endtransfer)
 + conn[0].close()
 + return (fp, conn[1])
 def endtransfer(self):
 if not self.busy:
 return

Would it be possible to commit this partial fix now? It gets rid of 4 of the 8 warnings that I am currently seeing in test_urllib2net.
(As an aside, for anyone reading this who hasn't seen issue11563, test_urllibnet is now warning-free)

I saw the partial fix suggested by the patch, but for some reason I
did not see ResourceWarning being shutup. Let's look at this again.
The warnings are all from the (old) ftp portion of the code..

> I saw the partial fix suggested by the patch, but for some reason I
> did not see ResourceWarning being shutup.
Do you mean that you aren't getting ResourceWarnings in the first place?
Or that you are getting warnings, and the partial fix isn't getting rid
of any of them?
Note: it doesn't get rid of *all* of the warnings for test_urllib2net;
only some of them.
About the remaining warnings, it seems that FTPHandler.ftp_open() creates
an ftpwrapper object to connect to the server, and then the ftpwrapper
doesn't get closed anywhere. It has to stay open until the caller has
finished reading from the connection, so finding the right place to close
it might be a bit tricky.

New changeset 2e5aff2a9e54 by Senthil Kumaran in branch '3.2':
issue10883 - Silence some ftp related ResourceWarnings in test_urllib2net. Patch by Nadeem Vawda.
http://hg.python.org/cpython/rev/2e5aff2a9e54
New changeset 0937b3618b86 by Senthil Kumaran in branch 'default':
issue10883 - Silence some ftp related ResourceWarnings in test_urllib2net. Patch by Nadeem Vawda
http://hg.python.org/cpython/rev/0937b3618b86 

Is there still something to do in this issue? The initial report is fixed.

Yes, the fix I provided only eliminated some of the warnings. As of fd6446a88fe3, test_urllib2net still leaks 5 sockets.

New changeset ca18f7f35c28 by Victor Stinner in branch '3.2':
Issue #10883: test_urllib2net closes socket explicitly
http://hg.python.org/cpython/rev/ca18f7f35c28
New changeset 6d38060f290c by Victor Stinner in branch 'default':
(Merge 3.2) Issue #10883: test_urllib2net closes socket explicitly
http://hg.python.org/cpython/rev/6d38060f290c 

ftp_close.patch:
 - (in passive mode) FTP.ntransfercmd() closes explicitly the socket on error: the caller has not access to the socket on error
 - OtherNetworkTests of test_urllib2net clears CacheFTPHandler cache: add a CacheFTPHandler.clear_cache() for that (I didn't document this new method because other methods are also not documented)
 - the last change is the worst one (ugly hack): FTPHandler.ftp_open() changes the close callback of the addclosehook object in ftpwrapper.retrfile(), but not in CacheFTPHandler.
I don't like the implementation of the last change:
 - it adds a protected class attribute
 - ftpwrapper.retrfile() requires an explicit close=True argument: it would be better to use a "reference counter" (or something like that), like socket._io_refs

Here's an updated patch implementing reference counting for ftpwrapper.
It changes the semantics of ftpwrapper.close() to postpone actually closing
the connection until all files have also been closed (like socket.close()).

With the patch applied, test_urllib2net fails at test_ftp test case
when a valid and invalid url are presented in sequence. I think test
needs a change or a further look is needed at the patch.

The failure seems to occur sporadically. I'm looking into it.

The problem seems to be that CacheFTPHandler inherits ftp_open() from
FTPHandler - FTPHandler.ftp_open() marks the ftpwrapper object to be closed as
soon as the current transfer is complete. So CacheFTPHandler's cache ends up
full of closed ftpwrappers. I don't have time to put together a solution now,
but I'll work on something over the weekend.
Another thing: CacheFTPHandler.clear_cache() sometimes breaks the cache,
because it fails to clear self.timeout. Is there any reason why the timeouts
need to be in a separate dict from the cached connections themselves? It seems
like a very ugly and error-prone way of organizing things.

Updated patch with fixed refcounting mechanism. Also fixes clear_cache() in
CacheFTPWrapper to leave the cache in a consistent state for subsequent use.

New changeset c741ba9e37ef by Nadeem Vawda in branch '3.2':
Issue #10883: Fix socket leaks in urllib.request.
http://hg.python.org/cpython/rev/c741ba9e37ef
New changeset d68765bd6490 by Nadeem Vawda in branch 'default':
Merge: #10883: Fix socket leaks in urllib.request.
http://hg.python.org/cpython/rev/d68765bd6490 

New changeset dbf1e1a27427 by Nadeem Vawda in branch '2.7':
Issue #10883: Fix socket leaks in urllib.request.
http://hg.python.org/cpython/rev/dbf1e1a27427 

Thanks for your patch. ResourceWarning are really useful!