Skip to content

/cpython

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Dec 31, 2019
Merged

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

merged 2 commits into from Dec 31, 2019
+46 −1

Conversation

@orsenthil
Copy link
Member

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

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
@orsenthil orsenthil force-pushed the orsenthil:issue27973 branch from de7d414 to 8a0f652 Apr 9, 2017
@orsenthil orsenthil requested review from serhiy-storchaka and benjaminp and removed request for serhiy-storchaka Apr 10, 2017
@serhiy-storchaka
serhiy-storchaka reviewed Apr 10, 2017
View changes
Misc/NEWS Outdated
@@ -42,6 +42,8 @@ Extension Modules
Library
-------

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

This comment has been minimized.

Sign in to view
Copy link
@serhiy-storchaka

serhiy-storchaka Apr 10, 2017

Member

Missed :. The second - is not needed.
Lib/test/test_urllibnet.py Outdated
try:
for _ in range(3):
fp = urllib.urlopen(self.FTP_TEST_FILE)
# closing fp is not required.

This comment has been minimized.

Sign in to view
Copy link
@serhiy-storchaka

serhiy-storchaka Apr 10, 2017

Member

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.
Lib/test/test_urllibnet.py Outdated
for _ in range(3):
urllib.FancyURLopener().retrieve(
self.FTP_TEST_FILE,
tempfile.NamedTemporaryFile().name)

This comment has been minimized.

Sign in to view
Copy link
@serhiy-storchaka

serhiy-storchaka Apr 10, 2017

Member

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

This comment has been minimized.

Sign in to view
Copy link

doctoryes commented 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.
@Mariatta Mariatta added needs rebase and removed needs rebase labels Oct 9, 2017
@benjaminp benjaminp force-pushed the python:2.7 branch from 42b480c to 01a0fd4 Apr 14, 2018
@ZackerySpytz
ZackerySpytz reviewed Sep 8, 2019
View changes
Lib/test/test_urllibnet.py Outdated
@@ -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

This comment has been minimized.

Sign in to view
Copy link
@ZackerySpytz

ZackerySpytz Sep 8, 2019

Contributor

It seems that this TODO is no longer needed.
Lib/test/test_urllibnet.py Outdated

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

This comment has been minimized.

Sign in to view
Copy link
@ZackerySpytz

ZackerySpytz Sep 8, 2019

Contributor

for _ in range(3): is used a couple of times in this class. I think the 3 should be replaced with a constant.
Lib/test/test_urllibnet.py Outdated
fp = urllib.urlopen(self.FTP_TEST_FILE)
# closing fp is not required.
except IOError as e:
self.fail("Failed FTP binary file open."

This comment has been minimized.

Sign in to view
Copy link
@ZackerySpytz

ZackerySpytz Sep 8, 2019

Contributor

There should be a space after this period (or a space just before the "Error" on the next line).
@benjaminp
benjaminp requested changes Sep 12, 2019
View changes
Copy link
Contributor

benjaminp left a comment

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

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Sep 12, 2019

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
@orsenthil orsenthil force-pushed the orsenthil:issue27973 branch from 8a0f652 to 9ee4612 Dec 30, 2019
@orsenthil 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 orsenthil force-pushed the orsenthil:issue27973 branch from 3340a4e to e4c4046 Dec 30, 2019
@orsenthil

This comment has been minimized.

Sign in to view
Copy link
Member Author

orsenthil commented 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

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Dec 30, 2019

Thanks for making the requested changes!

@benjaminp: please review the changes made to this pull request.
@bedevere-bot bedevere-bot requested a review from benjaminp Dec 30, 2019
orsenthil added 2 commits Dec 30, 2019
@orsenthil
bpo-27973: Fix urllib.urlretrieve failing on subsequent ftp transfers…
c080f29 
… from the same host.
@orsenthil
bpo-35411: Skip test_urllibnet FTP tests on Travis CI.
Loading status checks…
3b16daf
@orsenthil orsenthil force-pushed the orsenthil:issue27973 branch from e4c4046 to 3b16daf Dec 30, 2019
@benjaminp
benjaminp approved these changes Dec 31, 2019
View changes
Copy link
Contributor

benjaminp left a comment

Since this is essentially a revert, it seems good.
@orsenthil orsenthil merged commit f82e59a into python:2.7 Dec 31, 2019
4 checks passed
4 checks passed
bedevere/issue-number Issue number 27973 found
Details
bedevere/maintenance-branch-pr Valid maintenance branch PR title.
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@orsenthil orsenthil deleted the orsenthil:issue27973 branch Dec 31, 2019
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Dec 31, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member

pablogsal commented 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka

@ZackerySpytz ZackerySpytz

@benjaminp benjaminp

Assignees
No one assigned
Labels
CLA signed
Projects
None yet
Milestone
No milestone
10 participants
@orsenthil @doctoryes @bedevere-bot @pablogsal @benjaminp @serhiy-storchaka @ZackerySpytz @brettcannon @Mariatta @the-knights-who-say-ni
You can’t perform that action at this time.