Author orsenthil
Recipients Sohaib Ahmad, gson, orsenthil, r.david.murray, scoder
Date 2017-01-22.17:12:07
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1485105128.58.0.422081983841.issue27973@psf.upfronthosting.co.za>
In-reply-to
Content
I spent time digging for a proper fix for this issue.

I've come to a conclusion that this commit, https://hg.python.org/cpython/rev/44d02a5d59fb (10 May 2016) in 2.7.12, was a mistake and needs to be reverted.

The reason this change was made was apparently, it fixed a particular problem with retrieving files in 3.x, and the change was backported (issue26960). It was reported and fixed in 3.x code line here http://bugs.python.org/issue16270 (This is an improper change too and it needs to be reverted, we will come to it at the end [3].

Why is this a problem?

1. The change made was essentially taking the logic of draining ftp response from endtransfer method. Historically, in the module, endtransfer() has been used before closing the connection and it is correct to drain response (voidresp() method call).

2. If we don't drain responses, we end up with problems like the current (issue27973) bug report.

3. The problem with issue16270 was the fail transfer failed only when urllopen was used as a context manager (which calls close implicitly on the same host). But ftp scenarios were designed for reusing the same host without calling close from a long time (3a) and context manager scenario is not applicable to 2.7 code (3b).

Here are some references on how the module shaped up:
 
3a. https://hg.python.org/cpython/rev/6e0eddfa404a - it talks about repeated retriving of the same file from host without closing as a feature.

3b. The urllopen context manager was applicable only in 3.x so the original problem of issue16270 was not reproducible in 2.7 and it was improper to backport those changes (issue26960). Even issue16270 it is improper to change the code in endtransfer, because the problem is specific with context-manager usecase of urlopen, regular usage is just fine.

This patch fixes the problem for 2.7. I have included tests in test_urllibnet to cover the scenarios that were reported. Please review this.

For 3.x code, I will reopen issue16270, I will port this patch with test cases and an additional case for context manager scenario.
History
Date User Action Args
2017-01-22 17:12:08orsenthilsetrecipients: + orsenthil, scoder, r.david.murray, Sohaib Ahmad, gson
2017-01-22 17:12:08orsenthilsetmessageid: <1485105128.58.0.422081983841.issue27973@psf.upfronthosting.co.za>
2017-01-22 17:12:08orsenthillinkissue27973 messages
2017-01-22 17:12:07orsenthilcreate