Skip to content

gh-81403: Fix for CacheFTPHandler in urllib #13951

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

Merged
merged 3 commits into from
Apr 23, 2023
Merged

Conversation

hemberger
Copy link
Contributor

@hemberger hemberger commented Jun 10, 2019

A call to FTP.ntransfercmd must be followed by FTP.voidresp to clear
the "end transfer" message. Without this, the client and server get
out of sync, which will result in an error if the FTP instance is
reused to open a second URL. This scenario occurs for even the most
basic usage of CacheFTPHandler.

Reverts the patch merged as a resolution to bpo-16270 and adds a test
case for the CacheFTPHandler in test_urllib2net.py.

https://bugs.python.org/issue37222

A call to FTP.ntransfercmd must be followed by FTP.voidresp to clear
the "end transfer" message. Without this, the client and server get
out of sync, which will result in an error if the FTP instance is
reused to open a second URL. This scenario occurs for even the most
basic usage of CacheFTPHandler.

Reverts the patch merged as a resolution to bpo-16270 and adds a test
case for the CacheFTPHandler in test_urllib2net.py.
@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Our records indicate we have not received your CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

@bedevere-bot
Copy link

Every change to Python requires a NEWS entry.

Please, add it using the blurb_it Web app or the blurb command-line tool.

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@arhadthedev arhadthedev changed the title bpo-37222: Fix for CacheFTPHandler in urllib gh-81403: Fix for CacheFTPHandler in urllib Apr 23, 2023
@orsenthil
Copy link
Member

This change looks good to me. This fell out of attention due to lack of news and I cannot edit this PR. I will merge this and make a minor change and NEWS as a separate PR commit.

The NEWS entry I will add is

Fix ftplib endtransfer method to call void response. Fixes the issue when ftp url is reused or with CacheFTPHandler.

Copy link
Member

@orsenthil orsenthil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@arhadthedev
Copy link
Member

arhadthedev commented Apr 23, 2023

Fix ftplib endtransfer method to call void response. Fixes the issue when ftp url is reused or with CacheFTPHandler.

Maybe it's worth copying the issue text? Like:

:class:`urllib.request.CacheFTPHandler` no longer raises :class:`URLError` if a cached FTP instance is reused.

@orsenthil
Copy link
Member

@arhadthedev - sounds good to me.

@arhadthedev
Copy link
Member

arhadthedev commented Apr 23, 2023

It seems automerge was ignored.

@miss-islington
Copy link
Contributor

Thanks @hemberger for the PR, and @orsenthil for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 23, 2023
bpo-37222: Fix for CacheFTPHandler in urllib

A call to FTP.ntransfercmd must be followed by FTP.voidresp to clear
the "end transfer" message. Without this, the client and server get
out of sync, which will result in an error if the FTP instance is
reused to open a second URL. This scenario occurs for even the most
basic usage of CacheFTPHandler.

Reverts the patch merged as a resolution to bpo-16270 and adds a test
case for the CacheFTPHandler in test_urllib2net.py.

(cherry picked from commit e38bebb)

Co-authored-by: Dan Hemberger <[email protected]>
Co-authored-by: Senthil Kumaran <[email protected]>
@bedevere-bot
Copy link

GH-103705 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Apr 23, 2023
orsenthil added a commit that referenced this pull request Apr 23, 2023
* gh-81403: Fix for CacheFTPHandler in urllib (GH-13951)

bpo-37222: Fix for CacheFTPHandler in urllib

A call to FTP.ntransfercmd must be followed by FTP.voidresp to clear
the "end transfer" message. Without this, the client and server get
out of sync, which will result in an error if the FTP instance is
reused to open a second URL. This scenario occurs for even the most
basic usage of CacheFTPHandler.

Reverts the patch merged as a resolution to bpo-16270 and adds a test
case for the CacheFTPHandler in test_urllib2net.py.

(cherry picked from commit e38bebb)

Co-authored-by: Dan Hemberger <[email protected]>
Co-authored-by: Senthil Kumaran <[email protected]>

* Added NEWS entry.

---------

Co-authored-by: Dan Hemberger <[email protected]>
Co-authored-by: Senthil Kumaran <[email protected]>
orsenthil added a commit to orsenthil/cpython that referenced this pull request Apr 23, 2023
orsenthil added a commit that referenced this pull request Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants