Opened 6 years ago

Closed 4 years ago

#3462 enhancement closed fixed (fixed)

FTP server: upload should support async close

Reported by: warner Owned by:
Priority: highest Milestone:
Component: ftp Keywords:
Cc: exarkun, terrycojones, spiv Branch: branches/ftp-async-close-3462
(diff, github, buildbot, log)
Author: warner Launchpad Bug:

Description

As discussed on the mailing list, the FTP server in current trunk (r24958) does not have a mechanism for handling uploads that are not complete the moment the last byte is accepted by the consumer. This makes it difficult, for example, to implement an FTP frontend on a virtual filesystem that takes some amount of time/work to finish the upload process.

The proposed solution is to enhance ftp:IWriteFile to add a close() method, which will be called after the consumer has been disconnected, but before the '226 Transfer Complete' response is sent. close() can return a Deferred, and the response will not be sent until after it fires. The value produced by this Deferred is ignored.

Other proposed solutions were:

  • modify unregisterProducer to allow it to return a Deferred
  • do something at the VFS level

A patch to implement this change is attached. It does not yet have specific test coverage, but of course the existing tests still pass.

Attachments (3)

t3462.diff (1.8 KB) - added by warner 6 years ago.
patch to implement the interface+implementation change
t3462-2.diff (5.2 KB) - added by warner 4 years ago.
new patch, adds tests
t3462-3.diff (5.7 KB) - added by warner 4 years ago.
new patch incorporating spiv's recommendations

Download all attachments as: .zip

Change History (14)

Changed 6 years ago by warner

patch to implement the interface+implementation change

comment:1 Changed 6 years ago by exarkun

  • Cc exarkun added
  • Owner changed from itamarst to warner

Seems like a good start. Can you add unit tests?

Changed 4 years ago by warner

new patch, adds tests

comment:2 Changed 4 years ago by warner

  • Cc terrycojones added
  • Keywords review added
  • Priority changed from normal to highest

ok, new patch uploaded with unit tests. exarkun gave it a quick once-over and didn't scream.

comment:3 Changed 4 years ago by terrycojones

  • Cc spiv added

I applied the latest path, examined the diff and the patched code, didn't see anything to object to. Ran trial twisted without problem.

Thanks for the beer, Brian.

comment:4 Changed 4 years ago by spiv

  • Keywords review removed

I like this idea, and this implementation of it. Just a couple of small nits:

  1. Given that you're reformatting the test_write docstring anyway, fix the typo of "with fires" (should be "which fires").
  2. I think docstrings are preferred to comments for test descriptions (even though test_ftp is pretty confused on this point). So change the initial comment of FTPCloseTest.test_write to a docstring.
  3. (Optional: perhaps a docstring on FTPCloseTest itself would be a good idea too, something like "Tests that the server invokes IWriteFile.close.")
  4. This needs a news file.

Changed 4 years ago by warner

new patch incorporating spiv's recommendations

comment:5 Changed 4 years ago by spiv

  • Keywords review added

Looks like warner forgot to reinstate the 'review' keyword.

comment:6 Changed 4 years ago by spiv

  • Owner changed from warner to spiv

comment:7 Changed 4 years ago by spiv

  • Keywords review removed

Looks ok to me. I'll put it on a branch and see what the buildbot thinks.

comment:8 Changed 4 years ago by spiv

  • Author set to spiv
  • Branch set to branches/ftp-async-close-3462

(In [28450]) Branching to ftp-async-close-3462.

comment:9 Changed 4 years ago by spiv

  • Author changed from spiv to warner

comment:10 Changed 4 years ago by spiv

  • Resolution set to fixed
  • Status changed from new to closed

(In [28453]) Merge ftp-async-close-3462

Author: warner
Reviewer: spiv
Fixes: #3462

twisted.protocol.ftp.IWriteFile now has a close() method, which can return a
Deferred. Previously a STOR command would finish immediately upon the receipt
of the last byte of the uploaded file. With close(), the backend can delay
the finish until it has performed some other slow action (like storing the
data to a virtual filesystem).

comment:11 Changed 3 years ago by <automation>

  • Owner spiv deleted
Note: See TracTickets for help on using tickets.