classification
Title: urllib hangs when closing connection
Type: behavior Stage: resolved
Components: Extension Modules Versions: Python 3.2, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: giampaolo.rodola, neologix, orsenthil, python-dev, rg3
Priority: normal Keywords: patch

Created on 2011-02-12 13:00 by rg3, last changed 2012-03-15 20:28 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
test.py rg3, 2011-02-12 13:00 Minimal case that reproduces the problem
urllib_ftp_close_27.diff neologix, 2011-02-22 19:56 Patch without wait for 2.7 review
urllib_ftp_close.diff neologix, 2011-02-22 19:57 Patch without wait for 3k review
Messages (11)
msg128444 - (view) Author: (rg3) Date: 2011-02-12 13:00
If you run the attached program, you can see the program hangs in the connection close stage. Uncommenting the sleep line makes the program work, so I suspect some kind of race condition.

The URL used belongs to a Slackware Linux mirror. I have not been able to reproduce this problem when using a different FTP mirror or using HTTP mirrors. 

The remote server seems to be using pure-ftpd. I have built the software and tested on localhost, but I could not reproduce the problem either.
msg128919 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-02-20 20:24
The problem is due to the way urllib closes a FTP data transfer.
The data channel is closed, and a shutdown hook is called that waits for a message on the control channel. But in that case, when the data connection is closed while the transfer is in progress, the server doesn't send any further message on the control channel, and we remain stuck (note that if the data channel is closed after the file has been transfered, in that case an end of transfer message is sent, which explains why this dones't happen with the sleep in between).
The solution is to first wait for a message on the control channel, and then close the data channel (which makes sense, a close hook is generally called before closing the corresponding connection).
The attached patch just does that.
Note that I'm not sure why we need to wait for a further message on the control channel (maybe it's part of an RFC or something...).
msg128978 - (view) Author: (rg3) Date: 2011-02-21 18:44
That makes sense and explains why the problem could not be reproduced over the loopback (the transfer would be too fast).

I have not tested the patch, but I can reproduce the problem with a local connection if I compile pure-ftpd with the --with-throttling switch and limit the bandwidth to 1 KB/sec, using the -t option.
msg128980 - (view) Author: (rg3) Date: 2011-02-21 18:58
I just tested the patch under Python 2.6. It doesn't seem to solve the problem.
msg128999 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-02-21 21:32
> I just tested the patch under Python 2.6. It doesn't seem to solve the problem. 

Are you sure the patch applied cleanly ?
I tested both on 3.2 and 2.7, and it fixed the problem for me.
If not, could you submit a tcpdump capture ?
msg129005 - (view) Author: (rg3) Date: 2011-02-21 22:26
I have to correct myself. I applied the patch manually to my Python 2.6 installation. In Python 2.6, the line you moved is number 961, and I did the same change.

With your change, the connection can be closed, but you have to wait for the file to be completely transferred. As I was throttling to 1 KB/sec initially, I thought it was still hanging because it takes more than 1 minute for the test file to be sent. Still, the connection isn't immediately closed when you request to close it.
msg129040 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-02-22 07:40
> rg3 <sarbalap+freshmeat@gmail.com> added the comment:
>
> I have to correct myself. I applied the patch manually to my Python 2.6 installation. In Python 2.6, the line you moved is number 961, and I did the same change.

OK. For information, you can apply it using the Unix "patch" command,
who can most of the time do all the work for you.

>
> With your change, the connection can be closed, but you have to wait for the file to be completely transferred. As I was throttling to 1 KB/sec initially, I thought it was still hanging because it takes more than 1 minute for the test file to be sent. Still, the connection isn't immediately closed when you request to close it.

That's expected, it's a consequence of this point I raised earlier:

> Note that I'm not sure why we need to wait for a further message on the control channel (maybe it's part of an RFC or something...).

The current code explicitely waits for the end of transfer before
closing the data channel.
Don't ask me why, I don't have a clue. I wrote a 2-line patch to
disable this behaviour which seems to work fine, but since I'm not
sure why the code is doing this right now, I'd like some feedback
before doing the change.
msg129110 - (view) Author: (rg3) Date: 2011-02-22 19:09
> > I have to correct myself. I applied the patch manually to my Python
> > 2.6 installation. In Python 2.6, the line you moved is number 961,
> > and I did the same change.
> 
> OK. For information, you can apply it using the Unix "patch" command,
> who can most of the time do all the work for you.

Yes, thanks, I already knew about "patch" but decided to apply the 
change manually just in case, as it belonged to a different Python 
branch.

> That's expected, it's a consequence of this point I raised earlier:
> > Note that I'm not sure why we need to wait for a further message on
> > the control channel (maybe it's part of an RFC or something...).

There's no doubt that not hanging is an improvement, but the current 
behavior somewhat defeats the purpose of an early call to "close".

To get a broader view, the test case I provided is actually part of a 
much larger program. In that program, I read lines from the Slackware 
change log, stopping when I read a line that the program already knows 
about (because it caches the change log contents). This prevents the 
program from reading more than a single line if no new entries are 
present in the change log, but having to wait for the full file defeats 
the purpose, specially on slow dial-up connections.
msg129115 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-02-22 19:57
Attached are two new versions which don't wait for the end of transfer.
msg129116 - (view) Author: (rg3) Date: 2011-02-22 20:14
Charles-Francois Natali, Tuesday, February 22, 2011 20:57:
> Attached are two new versions which don't wait for the end of
> transfer.

I tested the one for Python 2.7 applied to Python 2.6 and it appears to 
work perfectly. Thanks for the quick fix!
msg155953 - (view) Author: Roundup Robot (python-dev) Date: 2012-03-15 20:28
New changeset 6ce4868861ba by Senthil Kumaran in branch '2.7':
Fix the urllib closing issue which hangs on particular ftp urls/ftp servers. closes issue11199
http://hg.python.org/cpython/rev/6ce4868861ba

New changeset 891184abbf6e by Senthil Kumaran in branch '3.2':
closes Issue #11199: Fix the with urllib which hangs on particular ftp urls.
http://hg.python.org/cpython/rev/891184abbf6e

New changeset 9e7374779e19 by Senthil Kumaran in branch 'default':
port from 3.2 - Fix the urllib closing issue which hangs on particular ftp urls/ftp servers. closes issue11199
http://hg.python.org/cpython/rev/9e7374779e19
History
Date User Action Args
2012-03-15 20:28:48python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg155953

resolution: fixed
stage: patch review -> resolved
2011-02-22 20:14:34rg3setnosy: orsenthil, giampaolo.rodola, rg3, neologix
messages: + msg129116
2011-02-22 19:57:33neologixsetfiles: + urllib_ftp_close.diff
nosy: orsenthil, giampaolo.rodola, rg3, neologix
messages: + msg129115
2011-02-22 19:56:31neologixsetfiles: + urllib_ftp_close_27.diff
nosy: orsenthil, giampaolo.rodola, rg3, neologix
2011-02-22 19:55:56neologixsetfiles: - urllib_ftp_close_27.diff
nosy: orsenthil, giampaolo.rodola, rg3, neologix
2011-02-22 19:55:49neologixsetfiles: - urllib_ftp_close.diff
nosy: orsenthil, giampaolo.rodola, rg3, neologix
2011-02-22 19:09:55rg3setnosy: orsenthil, giampaolo.rodola, rg3, neologix
messages: + msg129110
2011-02-22 07:40:28neologixsetnosy: orsenthil, giampaolo.rodola, rg3, neologix
messages: + msg129040
2011-02-21 22:26:12rg3setnosy: orsenthil, giampaolo.rodola, rg3, neologix
messages: + msg129005
2011-02-21 21:32:42neologixsetnosy: orsenthil, giampaolo.rodola, rg3, neologix
messages: + msg128999
2011-02-21 18:58:55rg3setnosy: orsenthil, giampaolo.rodola, rg3, neologix
messages: + msg128980
2011-02-21 18:44:36rg3setnosy: orsenthil, giampaolo.rodola, rg3, neologix
messages: + msg128978
2011-02-20 21:04:04neologixsetfiles: + urllib_ftp_close_27.diff
nosy: orsenthil, giampaolo.rodola, rg3, neologix
2011-02-20 20:30:51pitrousetnosy: + giampaolo.rodola, orsenthil
stage: patch review

versions: + Python 2.7, Python 3.2, Python 3.3, - Python 2.6
2011-02-20 20:24:56neologixsetfiles: + urllib_ftp_close.diff

nosy: + neologix
messages: + msg128919

keywords: + patch
2011-02-12 13:00:03rg3create