Issue 2628: ftplib Persistent data connection (original) (raw)

Issue2628

Created on 2008-04-13 23:50 by jbell, last changed 2022-04-11 14:56 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
debug.log jbell,2008-04-13 23:54 Expected debug output
ftplib.py.blockmode.patch jbell,2008-05-12 21:24 Patch (revised) to add block mode review
ftplib.rst.blockmode.patch jbell,2008-05-12 21:25 Documentation patch review
Pull Requests
URL Status Linked Edit
PR 29337 closed jbell,2021-10-30 23:59
Messages (20)
msg65454 - (view) Author: Jonathan Bell (jbell) * Date: 2008-04-13 23:50
About a year ago I found myself fighting a broken FTP server that couldn't handle multiple passive data transfers through a firewall or NATed connection. Thankfully, this same problem server supports block transmission mode, which allows a client to create a single data connection for transferring multiple files. I've attached a patch (and sample debug output) for the latest trunk. Might this be useful to anyone else in any way? I realize any MODE option rather than S is widely unsupported and possibly an edge case, but getting this into trunk would be preferable. We've been running this code under Python2.3 for nearly a year -- the jobs run several times per hour -- and are extremely happy with the results.
msg65455 - (view) Author: Jonathan Bell (jbell) * Date: 2008-04-13 23:54
Here's the debug output when the transfers are going well.
msg66305 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2008-05-06 07:23
rather than using the array.array for your three byte header with manual parsing, please use struct.unpack. Whats a good way to test that this works? It'd be nice to have a unit test (a test_ftplib_net.py perhaps?) though I realize ftplib currently has poor test coverage with no such explicit network test existing beyond things the urllib tests might do. also, could you make the appropriate documentation updates against python trunk's Doc/lib/ftplib.rst. Mention when & what servers you found using this mode useful with if at all possible.
msg66762 - (view) Author: Jonathan Bell (jbell) * Date: 2008-05-12 21:47
I've attached two new files. The first swaps the array.array usage for struct.unpack. The second simply modifies the rst documentation. I'm not sure how we'd do any tests for FTP without making use of an actual server. In a quick check of servers, MadGoat (for OpenVMS) was the only BLOCK-supporting server I found; neither vsftpd nor proftpd support BLOCK. (I didn't check wuftpd.) Sadly, I've no publicly accessible servers available to me for others to test against.
msg76248 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2008-11-22 14:26
An actual test suite for ftplib is now available. Should we reconsider revamping this issue?
msg76293 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2008-11-24 01:54
sure go for it, i haven't had time to look at the latest patch myself.
msg76336 - (view) Author: Jonathan Bell (jbell) * Date: 2008-11-24 16:44
Yeah, I'm glad to see a test suite. I've only skimmed the test, but it seems like an excellent starting point. Initial thoughts for updating the tests: - Need a cmd_mode to handle the MODE command. - Suite cmd_retr logic needs to keep dtp connection open and write a simple header depending on MODE. Due to the Thanksgiving holiday, I may be without network access (or time) until next week, however.
msg404920 - (view) Author: mike mcleod (mikecmcleod) * Date: 2021-10-24 08:59
Hi, I would like to help on this issue.
msg405014 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2021-10-26 00:36
We don't have a CLA from jbell. I've sent an email asking him to do so... we'll see what happens.
msg405036 - (view) Author: mike mcleod (mikecmcleod) * Date: 2021-10-26 12:05
Hi Ethan, Thanks, awaiting reply.. Regards, Mike On Tue, 26 Oct 2021 at 01:36, Ethan Furman <report@bugs.python.org> wrote: > > Ethan Furman <ethan@stoneleaf.us> added the comment: > > We don't have a CLA from jbell. I've sent an email asking him to do so... > we'll see what happens. > > ---------- > nosy: +ethan.furman > > _______________________________________ > Python tracker <report@bugs.python.org> > <https://bugs.python.org/issue2628> > _______________________________________ >
msg405386 - (view) Author: Jonathan Bell (jbell) * Date: 2021-10-30 21:23
The CLA is signed, and I'm again able to work on this. I was able to update this locally for Python 3 with a minimal test case. What specifically were you looking for?
msg405388 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2021-10-30 22:01
If you're familiar with git and GitHub, can you create a PR? Otherwise, an updated patch here and we'll work on getting it merged.
msg405402 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2021-10-31 13:32
Hello. I added some initial comments to the PR, but I'm sort of skeptical about this. It must be noted that: 1) very few FTP servers probably support this feature (https://en.wikipedia.org/wiki/File_Transfer_Protocol#Data_transfer_modes) 2) the specs are very old (RFC-959 is from 1985), and I doubt they've been upgraded in later RFCs. The fact that a header is sent *before every data block* seems inefficient (why not just send the file size once?), probably more inefficient that opening a new connection each time (unless files are small, I suppose). Was this tested against an actual FTP server(s)? If yes, which one(s)? IMO, it would be good if some actual research/testing is done first, to see how actual FTP server products implement this feature. Another thing to note is that the PR supports RETR (download) only, and not STOR (upload). Is this on purpose or does the original RFC/spec limits this functionality to RETR?
msg405407 - (view) Author: Jonathan Bell (jbell) * Date: 2021-10-31 16:46
This issue is 13 years old. The original 2008 patch was used in a production environment against an OpenVMS server identifying itself as MadGoat. That use case involved downloading documents only, and no write permission was available. Therefore the patch only supports RETR. See the debug.log file attached to this issue for the server interaction. I no longer have a need for BLOCK mode, and don't know what modern servers would support it. mikecmcleod revived this issue so perhaps they can provide some ability for testing, or perspective on the current needs. The PR updates the patch to Python 3, and includes a test written against the minimal changes required for that 2.7->3.x update.
msg405426 - (view) Author: mike mcleod (mikecmcleod) * Date: 2021-11-01 10:35
I am happy to do any testing. My reason for getting involved is I am new to helping with Cpython and thought this may be the least intrusive way of getting started with something that nobody really cares about that much. Hence, the oldest issue I see can be either completed as first envisioned or can be closed and I am ok with either.
msg405443 - (view) Author: Jonathan Bell (jbell) * Date: 2021-11-01 14:22
No practical method exists to verify BLOCK transmission mode, which as mentioned earlier, was rarely implemented even when this issue was opened. Given that reality, I'm inclined to close this issue.
msg405457 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2021-11-01 17:50
I would like to have the issue fixed instead of just closed. Jonathan, you say there is no practical method to verify that the block transmission mode exists -- so it's only useful if the user knows that it exists? If the user tries it and the server does not support it, is a useful exception raised? If the answer is yes (to the useful exception) I'm would like to see it added.
msg405480 - (view) Author: Jonathan Bell (jbell) * Date: 2021-11-02 03:14
I should rephrase: There doesn't seem to be a practical way to verify BLOCK transmission mode against actual servers in the wild. As the Wikipedia article that Giampaolo referenced points out, BLOCK mode is a rarity that was primarily supported only by mainframe and minicomputer systems. Any compliant server not supporting BLOCK should respond with a non-200 response. The PR sends its request to enter BLOCK mode with self.voidcmd(), which handles non-200 responses by raising error_reply. When I originally wrote that patch in 2008, such a system was running on a DEC Alpha under OpenVMS. Within months of the first test suite appearing for ftplib, that same vendor replaced their systems. The new server had no BLOCK transmission support, but was capable of handling multiple consecutive passive mode STREAM data connections without fault. Even at the time, I couldn't find any other freely available FTP servers supporting BLOCK. But STREAM was and continues to be the standard. Essentially this means that any changes to the existing PR may not be work properly with actual servers.
msg405492 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2021-11-02 05:52
Ah. Well, in that case closing seems like the best idea. Thank you, Jonathan, for getting the CLA signed and providing perspective. Thank you, Mike, for moving this issue forward.
msg405497 - (view) Author: mike mcleod (mikecmcleod) * Date: 2021-11-02 09:33
You're welcome. Regards, Mike On Tue, 2 Nov 2021 at 05:52, Ethan Furman <report@bugs.python.org> wrote: > > Ethan Furman <ethan@stoneleaf.us> added the comment: > > Ah. Well, in that case closing seems like the best idea. > > Thank you, Jonathan, for getting the CLA signed and providing perspective. > > Thank you, Mike, for moving this issue forward. > > ---------- > resolution: -> rejected > stage: patch review -> resolved > status: open -> closed > > _______________________________________ > Python tracker <report@bugs.python.org> > <https://bugs.python.org/issue2628> > _______________________________________ >
History
Date User Action Args
2022-04-11 14:56:33 admin set github: 46880
2021-11-02 09:33:31 mikecmcleod set messages: +
2021-11-02 05:52:28 ethan.furman set status: open -> closedresolution: rejectedmessages: + stage: patch review -> resolved
2021-11-02 03:14:17 jbell set messages: +
2021-11-01 17:50:35 ethan.furman set messages: + versions: + Python 3.11, - Python 3.2
2021-11-01 14:22:20 jbell set messages: +
2021-11-01 10:35:06 mikecmcleod set messages: +
2021-10-31 16:46:04 jbell set messages: +
2021-10-31 13:32:18 giampaolo.rodola set messages: +
2021-10-30 23:59:34 jbell set stage: test needed -> patch reviewpull_requests: + <pull%5Frequest27604>
2021-10-30 22:01:31 ethan.furman set messages: +
2021-10-30 21:23:57 jbell set messages: +
2021-10-26 12:05:24 mikecmcleod set messages: +
2021-10-26 00:36:45 ethan.furman set nosy: + ethan.furmanmessages: +
2021-10-24 08:59:30 mikecmcleod set nosy: + mikecmcleodmessages: +
2010-08-07 16:33:11 terry.reedy set stage: test needed
2010-08-07 16:22:17 terry.reedy set versions: + Python 3.2, - Python 2.6
2010-08-07 16:17:14 terry.reedy set files: - ftplib.py.blockmode.patch
2008-11-24 16:44:13 jbell set messages: +
2008-11-24 01:54:35 gregory.p.smith set messages: +
2008-11-22 14:26:27 giampaolo.rodola set messages: +
2008-05-12 21:47:35 jbell set messages: +
2008-05-12 21:25:13 jbell set files: + ftplib.rst.blockmode.patch
2008-05-12 21:24:47 jbell set files: + ftplib.py.blockmode.patch
2008-05-06 07:23:26 gregory.p.smith set nosy: + gregory.p.smithmessages: +
2008-04-14 15:09:51 giampaolo.rodola set nosy: + giampaolo.rodola
2008-04-13 23:54:53 jbell set files: + debug.logmessages: +
2008-04-13 23:50:10 jbell create