Ticket #3462 enhancement closed fixed

Opened 6 years ago

Last modified 4 years ago

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

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

Change History

Changed 6 years ago by warner

patch to implement the interface+implementation change

1

Changed 5 years ago by exarkun

  • owner changed from itamarst to warner
  • cc exarkun added

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

Changed 4 years ago by warner

new patch, adds tests

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.

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.

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

5

Changed 4 years ago by spiv

  • keywords review added

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

6

Changed 4 years ago by spiv

  • owner changed from warner to spiv

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.

8

Changed 4 years ago by spiv

  • branch set to branches/ftp-async-close-3462
  • branch_author set to spiv

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

9

Changed 4 years ago by spiv

  • branch_author changed from spiv to warner

10

Changed 4 years ago by spiv

  • status changed from new to closed
  • resolution set to fixed

(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).

11

Changed 3 years ago by <automation>

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