[2.7] bpo-27973 - Fix for urllib.urlretrieve() failing on second ftp transfer by orsenthil · Pull Request #1040 · python/cpython

orsenthil · Apr 8, 2017

The problem and the solution has been described well in the msg: http://bugs.python.org/issue27973#msg286016

This just brings the patch to github.

https://bugs.python.org/issue27973

serhiy-storchaka · Apr 10, 2017

 Library
 -------

+- bpo-27973 - Fix for urllib.urlretrieve() failing on second ftp transfer.


Missed :. The second - is not needed.

serhiy-storchaka · Apr 10, 2017

+            try:
+                for _ in range(3):
+                    fp = urllib.urlopen(self.FTP_TEST_FILE)
+                    # closing  fp is not required.


This test can behave differently in CPython and PyPy. If fp can remain unclosed, maybe save it in a list, so that it will not be closed in all implementations? If the file is not closed explicitly it is worth to call test_support.gc_collect() after removing all references to it. This will make the behavior more predicable in all implementations.

serhiy-storchaka · Apr 10, 2017

+                for _ in range(3):
+                    urllib.FancyURLopener().retrieve(
+                            self.FTP_TEST_FILE,
+                            tempfile.NamedTemporaryFile().name)


The NamedTemporaryFile object can be collected right after getting its name. Save a reference to tempfile.NamedTemporaryFile() until retrieve a file or better use a context manager.

doctoryes · Sep 13, 2017

Has this fix stalled? I'm able to reproduce this problem in Python 2.7.13 with the following steps:

Create a virtualenv.
STATIC_DEPS=true pip --no-cache-dir install lxml==3.4.4

These steps result in this error:

Collecting lxml==3.4.4
  Downloading lxml-3.4.4.tar.gz (3.5MB)
    100% |████████████████████████████████| 3.5MB 12.0MB/s
    Complete output from command python setup.py egg_info:
    Building lxml version 3.4.4.
    Latest version of libiconv is 1.15
    Downloading libiconv into libs/libiconv-1.15.tar.gz
    Unpacking libiconv-1.15.tar.gz into build/tmp
    Latest version of libxml2 is 2.9.5
    Downloading libxml2 into libs/libxml2-2.9.5.tar.gz
    Unpacking libxml2-2.9.5.tar.gz into build/tmp
    Latest version of libxslt is 1.1.30
    Downloading libxslt into libs/libxslt-1.1.30.tar.gz
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/private/var/folders/5c/jyk57yxn3zl9cyv5gmn84hxm0000gp/T/pip-build-LY1sdH/lxml/setup.py", line 230, in <module>
        **setup_extra_options()
      File "/private/var/folders/5c/jyk57yxn3zl9cyv5gmn84hxm0000gp/T/pip-build-LY1sdH/lxml/setup.py", line 144, in setup_extra_options
        STATIC_CFLAGS, STATIC_BINARIES)
      File "setupinfo.py", line 57, in ext_modules
        multicore=OPTION_MULTICORE)
      File "buildlibxml.py", line 333, in build_libxml2xslt
        libxslt_dir  = unpack_tarball(download_libxslt(download_dir, libxslt_version), build_dir)
      File "buildlibxml.py", line 129, in download_libxslt
        version_re, filename, version=version)
      File "buildlibxml.py", line 182, in download_library
        urlretrieve(full_url, dest_filename)
      File "/Users/jeskew/.pyenv/versions/2.7.13/lib/python2.7/urllib.py", line 98, in urlretrieve
        return opener.retrieve(url, filename, reporthook, data)
      File "/Users/jeskew/.pyenv/versions/2.7.13/lib/python2.7/urllib.py", line 245, in retrieve
        fp = self.open(url, data)
      File "/Users/jeskew/.pyenv/versions/2.7.13/lib/python2.7/urllib.py", line 213, in open
        return getattr(self, name)(url)
      File "/Users/jeskew/.pyenv/versions/2.7.13/lib/python2.7/urllib.py", line 558, in open_ftp
        (fp, retrlen) = self.ftpcache[key].retrfile(file, type)
      File "/Users/jeskew/.pyenv/versions/2.7.13/lib/python2.7/urllib.py", line 906, in retrfile
        conn, retrlen = self.ftp.ntransfercmd(cmd)
      File "/Users/jeskew/.pyenv/versions/2.7.13/lib/python2.7/ftplib.py", line 334, in ntransfercmd
        host, port = self.makepasv()
      File "/Users/jeskew/.pyenv/versions/2.7.13/lib/python2.7/ftplib.py", line 312, in makepasv
        host, port = parse227(self.sendcmd('PASV'))
      File "/Users/jeskew/.pyenv/versions/2.7.13/lib/python2.7/ftplib.py", line 830, in parse227
        raise error_reply, resp
    IOError: [Errno ftp error] 200 Switching to Binary mode.

ZackerySpytz · Sep 8, 2019



+class urlopen_FTPTest(unittest.TestCase):
+    # TODO: https://github.com/python/pythondotorg/issues/1069


It seems that this TODO is no longer needed.

ZackerySpytz · Sep 8, 2019

+
+        with test_support.transient_internet(self.FTP_TEST_FILE):
+            try:
+                for _ in range(3):


for _ in range(3): is used a couple of times in this class. I think the 3 should be replaced with a constant.

ZackerySpytz · Sep 8, 2019

+                    fp = urllib.urlopen(self.FTP_TEST_FILE)
+                    # closing  fp is not required.
+            except IOError as e:
+                self.fail("Failed FTP binary file open." 


There should be a space after this period (or a space just before the "Error" on the next line).

benjaminp

If you still want to pursue this, it will need to be rebased and blurbed.

bedevere-bot · Sep 12, 2019

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

orsenthil · Dec 30, 2019

I have made the requested changes; please review again.

@benjaminp - Please take a call to include the final 2.7 release. Thanks.

bedevere-bot · Dec 30, 2019

Thanks for making the requested changes!

@benjaminp: please review the changes made to this pull request.

… from the same host.

benjaminp

Since this is essentially a revert, it seems good.

bedevere-bot · Dec 31, 2019

@orsenthil: Please replace # with GH- in the commit message next time. Thanks!

pablogsal · Dec 31, 2019

This PR is failing in several 2.7 buildbots:

Tests result: FAILURE then FAILURE
test test_urllibnet failed -- Traceback (most recent call last):
  File "D:\cygwin\home\db3l\buildarea\2.7.bolen-windows7\build\lib\test\test_urllibnet.py", line 232, in test_multiple_ftp_retrieves
    "multiple times.\n Error message was : %s" % e)
AssertionError: Failed FTP retrieve while accessing ftp url multiple times.
 Error message was : [Errno 13] Permission denied: 'd:\\temp\\tmpiuquqa'

Example failure:

https://buildbot.python.org/all/#/builders/209/builds/4

the-knights-who-say-ni added the CLA signed label Apr 8, 2017

orsenthil requested review from benjaminp and serhiy-storchaka and removed request for serhiy-storchaka April 10, 2017 02:06

serhiy-storchaka reviewed Apr 10, 2017

View reviewed changes

Mariatta added needs rebase and removed needs rebase labels Oct 9, 2017

brettcannon added the awaiting review label Feb 21, 2018

benjaminp force-pushed the 2.7 branch from 42b480c to 01a0fd4 Compare April 14, 2018 22:00

ZackerySpytz reviewed Sep 8, 2019

View reviewed changes

benjaminp requested changes Sep 12, 2019

View reviewed changes

bedevere-bot removed the awaiting review label Sep 12, 2019

bedevere-bot added the awaiting changes label Sep 12, 2019

orsenthil changed the title ~~bpo-27973 - Fix for urllib.urlretrieve() failing on second ftp transfer~~ [2.7] bpo-27973 - Fix for urllib.urlretrieve() failing on second ftp transfer Dec 30, 2019

bedevere-bot added awaiting change review and removed awaiting changes labels Dec 30, 2019

bedevere-bot requested a review from benjaminp December 30, 2019 17:28

orsenthil added 2 commits December 30, 2019 10:02

bpo-27973: Fix urllib.urlretrieve failing on subsequent ftp transfers…

c080f29

… from the same host.

bpo-35411: Skip test_urllibnet FTP tests on Travis CI.

3b16daf

benjaminp approved these changes Dec 31, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Dec 31, 2019

orsenthil merged commit f82e59a into python:2.7 Dec 31, 2019

orsenthil deleted the issue27973 branch December 31, 2019 05:14

bedevere-bot removed the awaiting merge label Dec 31, 2019



		class urlopen_FTPTest(unittest.TestCase):
		# TODO: https://github.com/python/pythondotorg/issues/1069

Search code, repositories, users, issues, pull requests...

Uh oh!

Conversation

orsenthil commented Apr 8, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka Apr 10, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Apr 10, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Apr 10, 2017

Choose a reason for hiding this comment

Uh oh!

doctoryes commented Sep 13, 2017

Uh oh!

ZackerySpytz Sep 8, 2019

Choose a reason for hiding this comment

Uh oh!

ZackerySpytz Sep 8, 2019

Choose a reason for hiding this comment

Uh oh!

ZackerySpytz Sep 8, 2019

Choose a reason for hiding this comment

Uh oh!

benjaminp left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 12, 2019

Uh oh!

orsenthil commented Dec 30, 2019

Uh oh!

bedevere-bot commented Dec 30, 2019

Uh oh!

benjaminp left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Dec 31, 2019

Uh oh!

pablogsal commented Dec 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

orsenthil commented Apr 8, 2017 •

edited by bedevere-bot

Loading