Opened 7 years ago

Last modified 4 months ago

#4819 enhancement new

Add FTP support for REST and APPE

Reported by: Andrew Owned by: Julian Berman
Priority: normal Milestone:
Component: ftp Keywords:
Cc: Andrew, sirkonst Branch:
Author:

Description

Currently, the FTP server doesn't support the FTP REST command. Each incoming command is read, and pauseProducing is called before the command is processed. No other commands are read until the current command is finished.

Clients (I tested with filezilla) send a REST command after a PASV command and then wait for a response before making the DTP connection. FTP.ftp_PASV, however, waits for a DTP connection before resumeProducing is called (effectively). This would result in a deadlock between the two, if the current implementation didn't return "Not Implemented" for REST.

I've posted my workaround for this on StackOverflow: Implementing REST in tiwsted.protocols.ftp.FTP?. This sets the reactor to call resumeProducing in the near future with callLater, canceling that call if the DTP connection is made in the interim. This allows a REST command to come in and be processed if necessary, or normal processing to go on if not.

Attachments (1)

test-ftp-server-rest-appe.patch (5.7 KB) - added by Andrew 6 years ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 6 years ago by Andrew

More info: The instability I mention in the stackoverflow post is possibly due to the fact (?) that the callLater on resumeProducing results in two simultaneous flows of line processing. Adding a "peek" flag to prevent resuming (line 716) on a "peeking pass" seems to fix the issue and make REST stable.

Changed 6 years ago by Andrew

comment:2 Changed 6 years ago by Andrew

Cc: Andrew added
Keywords: ftp rest pasv removed
Summary: Add support for REST in t.p.ftp.FTPAdd FTP support for REST and APPE

In the interest of getting this started, I've attached a patch that adds unit tests for the server portion of APPE and REST (both marked todo), as well as one for STOR. I tried my darndest to write tests for the FTP client, too, but I'm simply not familiar enough with how it works. FTPShell may also need some tests.

Also, feedback on this patch is appreciated, since I'm still getting the hang of contributing twisted code.

comment:3 Changed 6 years ago by <automation>

Owner: itamarst deleted

comment:4 Changed 4 years ago by sirkonst

Cc: sirkonst added
Priority: normalhighest

ping

comment:5 Changed 4 months ago by Julian Berman

Keywords: review added
Priority: highestnormal

comment:6 Changed 4 months ago by Glyph

Keywords: review removed
Owner: set to Julian Berman

Hi Julian,

I'm not sure what's been submitted for review here. Is it... the 6-year-old patch? If so, could you open a PR with it? (It's got some pretty obvious coding-standard compliance issues so I expect you'll have mechanical stuff to fix before a review is available) Is there a PR or branch somewhere already that isn't linked?

Please make it clear what it is that's up for review and then resubmit. Thanks for spelunking through ticket history and finding a ticket with a patch though, I hope we can integrate this :).

Note: See TracTickets for help on using tickets.