[2.7] bpo-27973 - Fix for urllib.urlretrieve() failing on second ftp transfer #1040

orsenthil · 2017-04-08T05:06:53Z

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 · 2017-04-10T06:41:14Z

Misc/NEWS

@@ -42,6 +42,8 @@ Extension Modules
 Library
 -------

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


Missed :. The second - is not needed.

serhiy-storchaka · 2017-04-10T06:41:14Z

Lib/test/test_urllibnet.py

+            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 · 2017-04-10T06:41:14Z

Lib/test/test_urllibnet.py

+                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 · 2017-09-13T19:42:53Z

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 · 2019-09-08T05:42:34Z

Lib/test/test_urllibnet.py

@@ -213,14 +214,44 @@ def test_context_argument(self):
        self.assertIn("Python", response.read())


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


It seems that this TODO is no longer needed.

ZackerySpytz · 2019-09-08T05:42:34Z

Lib/test/test_urllibnet.py

+
+        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 · 2019-09-08T05:42:34Z

Lib/test/test_urllibnet.py

+                    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 · 2019-09-12T14:11:38Z

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

orsenthil · 2019-12-30T17:28:34Z

I have made the requested changes; please review again.

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

bedevere-bot · 2019-12-30T17:28:37Z

Thanks for making the requested changes!

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


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

… from the same host.


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

benjaminp

Since this is essentially a revert, it seems good.

bedevere-bot · 2019-12-31T05:14:59Z

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

pablogsal · 2019-12-31T19:55:54Z

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 force-pushed the orsenthil:issue27973 branch from de7d414 to 8a0f652 Apr 9, 2017

orsenthil requested review from serhiy-storchaka and benjaminp and removed request for serhiy-storchaka Apr 10, 2017

serhiy-storchaka reviewed Apr 10, 2017

View 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 python:2.7 branch from 42b480c to 01a0fd4 Apr 14, 2018

ZackerySpytz reviewed Sep 8, 2019

View changes

benjaminp requested changes Sep 12, 2019

View changes

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

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

orsenthil force-pushed the orsenthil:issue27973 branch from 8a0f652 to 9ee4612 Dec 30, 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

orsenthil force-pushed the orsenthil:issue27973 branch from 3340a4e to e4c4046 Dec 30, 2019

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

bedevere-bot requested a review from benjaminp Dec 30, 2019

orsenthil added 2 commits Dec 30, 2019

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.

Loading status checks…

3b16daf

orsenthil force-pushed the orsenthil:issue27973 branch from e4c4046 to 3b16daf Dec 30, 2019

benjaminp approved these changes Dec 31, 2019

View changes

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

orsenthil deleted the orsenthil:issue27973 branch Dec 31, 2019

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

python/cpython

[2.7] bpo-27973 - Fix for urllib.urlretrieve() failing on second ftp transfer #1040

[2.7] bpo-27973 - Fix for urllib.urlretrieve() failing on second ftp transfer #1040

orsenthil commented Apr 8, 2017 •

edited by bedevere-bot

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

doctoryes commented Sep 13, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

benjaminp left a comment

This comment has been minimized.

bedevere-bot commented Sep 12, 2019

This comment has been minimized.

orsenthil commented Dec 30, 2019

This comment has been minimized.

bedevere-bot commented Dec 30, 2019

benjaminp left a comment

This comment has been minimized.

bedevere-bot commented Dec 31, 2019

This comment has been minimized.

pablogsal commented Dec 31, 2019

Sponsor python/cpython

python/cpython

Join GitHub today

[2.7] bpo-27973 - Fix for urllib.urlretrieve() failing on second ftp transfer #1040

[2.7] bpo-27973 - Fix for urllib.urlretrieve() failing on second ftp transfer #1040

Conversation

orsenthil commented Apr 8, 2017 • edited by bedevere-bot

This comment has been minimized.

serhiy-storchaka Apr 10, 2017

This comment has been minimized.

serhiy-storchaka Apr 10, 2017

This comment has been minimized.

serhiy-storchaka Apr 10, 2017

This comment has been minimized.

doctoryes commented Sep 13, 2017

This comment has been minimized.

ZackerySpytz Sep 8, 2019

This comment has been minimized.

ZackerySpytz Sep 8, 2019

This comment has been minimized.

ZackerySpytz Sep 8, 2019

benjaminp left a comment

This comment has been minimized.

bedevere-bot commented Sep 12, 2019

This comment has been minimized.

orsenthil commented Dec 30, 2019

This comment has been minimized.

bedevere-bot commented Dec 30, 2019

benjaminp left a comment

This comment has been minimized.

bedevere-bot commented Dec 31, 2019

This comment has been minimized.

pablogsal commented Dec 31, 2019

orsenthil commented Apr 8, 2017 •

edited by bedevere-bot