classification
Title: urllib missing voidresp breaks CacheFTPHandler
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.9
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: danh, giampaolo.rodola, orsenthil
Priority: normal Keywords: patch

Created on 2019-06-10 23:38 by danh, last changed 2019-06-11 08:19 by xtreak.

Pull Requests
URL Status Linked Edit
PR 13951 open danh, 2019-06-10 23:44
Messages (1)
msg345155 - (view) Author: Dan Hemberger (danh) * Date: 2019-06-10 23:38
When using the CacheFTPHandler in the most basic of contexts, a URLError will be thrown if you try to reuse any of the FTP instances stored in the handler. This makes CacheFTPHandler unusable for its intended purpose. Note that the default FTPHandler circumvents this issue by creating a new ftplib.FTP instance for each connection (and thus never reuses any of them).

Here is a simple example illustrating the problem:

"""
import urllib.request as req 
import ftplib

opener = req.build_opener(req.CacheFTPHandler)
req.install_opener(opener)

ftplib.FTP.debugging = 2 

for _ in range(2):
    req.urlopen("ftp://www.pythontest.net/README", timeout=10)
"""

From the ftplib debugging output, we see the following communication between the client and server:

"""
*cmd* 'TYPE I'
*put* 'TYPE I\r\n'
*get* '200 Switching to Binary mode.\n'
*resp* '200 Switching to Binary mode.'
*cmd* 'PASV'
*put* 'PASV\r\n'
*get* '227 Entering Passive Mode (159,89,235,38,39,111).\n'
*resp* '227 Entering Passive Mode (159,89,235,38,39,111).'
*cmd* 'RETR README'
*put* 'RETR README\r\n'
*get* '150 Opening BINARY mode data connection for README (123 bytes).\n'
*resp* '150 Opening BINARY mode data connection for README (123 bytes).'
*cmd* 'TYPE I'
*put* 'TYPE I\r\n'
*get* '226 Transfer complete.\n'
*resp* '226 Transfer complete.'
*cmd* 'PASV'
*put* 'PASV\r\n'
*get* '200 Switching to Binary mode.\n'
*resp* '200 Switching to Binary mode.'
ftplib.error_reply: 200 Switching to Binary mode.
"""

The client and the server have gotten out of sync due to the missing voidresp() call, i.e. the client sends 'Type I' but receives the response from the previous 'RETR README' command. When ftp.voidresp() is added anywhere between after the ftp.ntransfercmd() and before the next command is sent (i.e. by reverting 2d51f687e133fb8141f1a6b5a6ac51c9d5eddf58), we see the correct send/receive pattern:

"""
*cmd* 'TYPE I'
*put* 'TYPE I\r\n'
*get* '200 Switching to Binary mode.\n'
*resp* '200 Switching to Binary mode.'
*cmd* 'PASV'
*put* 'PASV\r\n'
*get* '227 Entering Passive Mode (159,89,235,38,39,107).\n'
*resp* '227 Entering Passive Mode (159,89,235,38,39,107).'
*cmd* 'RETR README'
*put* 'RETR README\r\n'
*get* '150 Opening BINARY mode data connection for README (123 bytes).\n'
*resp* '150 Opening BINARY mode data connection for README (123 bytes).'
*get* '226 Transfer complete.\n'
*resp* '226 Transfer complete.'
*cmd* 'TYPE I'
*put* 'TYPE I\r\n'
*get* '200 Switching to Binary mode.\n'
*resp* '200 Switching to Binary mode.'
*cmd* 'PASV'
*put* 'PASV\r\n'
*get* '227 Entering Passive Mode (159,89,235,38,39,107).\n'
*resp* '227 Entering Passive Mode (159,89,235,38,39,107).'
*cmd* 'RETR README'
*put* 'RETR README\r\n'
*get* '150 Opening BINARY mode data connection for README (123 bytes).\n'
*resp* '150 Opening BINARY mode data connection for README (123 bytes).'
*get* '226 Transfer complete.\n'
*resp* '226 Transfer complete.'
"""

By inspecting the methods of ftplib.FTP, we can see that every use of ntransfercmd() is followed by a voidresp(), see e.g. retrbinary, retrlines, storbinary, storlines, and voidcmd.

I hope that some experts in urllib and ftplib can weigh in on any of the subtleties of this issue, but I think it's clear that the missing ftp.voidresp() call is a significant bug.

--------------------------------------
Some historical notes about this issue
--------------------------------------

This issue has been documented in a number of other bug reports, but I don't think any have addressed the complete breakage of the CachedFTPHandler that it causes.

The breaking change was originally introduced as a resolution to issue16270. However, it's not clear from the comments why it was believed that removing ftp.voidresp() from endtransfer() was the correct solution. In either case, with this commit reverted, both the test outlined in this report and in issue16270 work correctly.

@orsenthil has suggested this fix (to revert the change to endtransfer) in msg286020, and has explained his reasoning in detail in msg286016.

@Ivan.Pozdeev has also explained this issue in msg282797 and provided a similar patch in issue28931, though it does more than just revert the breaking commit and I'm not sure what the additional changes are intending to fix.
History
Date User Action Args
2019-06-11 08:19:30xtreaksetnosy: + orsenthil, giampaolo.rodola
2019-06-10 23:44:24danhsetkeywords: + patch
stage: patch review
pull_requests: + pull_request13819
2019-06-10 23:38:21danhcreate