Fix for mutable files with FTP

[frozenfire]

FTP hangs when a directory list contains mutable files, as their filesize is the string "?", and Twisted's ftpd expects an integer. This patch by warner should fix it.

For a different reason (comment:25), mutable files cannot then be downloaded successfully over FTP.

[frozenfire]

The patch, in diff/patch format

[zooko]

Actually it is the mutable files whose size shows up as "?". (This is because their size could change, and it would be expensive to examine each child to find out its current size when listing the parent directory.)

If only the code changed by this patch were covered by automated tests, I would consider including this patch in Tahoe-1.4.0. But in the interests of stability I don't want to apply patches to untested code just before the 1.4.0 release.

[warner]

I feel pretty comfortable about this patch. I'll try to apply it tonight. If 1.4.0 happens before then, I'd be ok with that too.

[frozenfire]

Hmm, this seems to misbehave with curlftpfs, will test with normal FTP client too. With curlftpfs it simply sees them as files of zero length, as the FTP server says. So they're unusable there.

[frozenfire]

Unusable in normal FTP client too D=

[warner]

Drat.. maybe the client is stubbornly remembering the "0" size, and refuses to download more than 0 bytes of it later on. A simple test of this would be to change that "if not a number, return 0" to "if not a number, return 1", and then see what the client does when you try to download the file.

Anyways, now I'm thinking we should *not* apply this patch before 1.4.0 .. better to keep the broken behavior the same, rather than switch it to a different broken behavior.

[zooko]

Should we add it to docs/known_issues.txt, with a suggested workaround saying "Don't use mutable files."?

[davidsarah]

FTP is a horrible protocol, and grokking its specification (RFC 959) is not helped by the fact that it has never been streamlined and updated for a world where bytes are always 8 bits and files are flat.

In any case, the relevant part of RFC 959 seems to be section 3.4, which says

The following transmission modes are defined in FTP:

3.4.1. STREAM MODE

If the structure is a file structure, the EOF is indicated by the sending host closing the data connection and all bytes are data bytes.

It is very unlikely that the FTP server library is using anything other than STREAM transmission mode with a file structure ( ​http://cr.yp.to/ftp/type.html says the other options are all obsolete). If so, then the tested clients are nonconformant in treating anything other than a close of the data connection as end-of-file. ​http://cr.yp.to/ftp/browsing.html seems to suggest that browser ftp clients do actually look for connection close to signal EOF -- so I'm stumped.

[davidsarah]

SFTP has a similar problem -- just writing up a new ticket about that. I think blackmatch fuse also gets this wrong.

[davidsarah]

Replying to zooko:

Should we add it to docs/known_issues.txt, with a suggested workaround saying "Don't use mutable files."?

Yes (more precisely, don't use mutable files with FTP). We should leave this ticket open since it might be fixable, even though I don't currently see how to do so reliably.

[zooko]

I guess this doesn't rise to the level of a Known Issue to be added to docs/known_issues.txt. (That file is more for things which might badly burn you if you don't know about them.) Let's add a note about this issue to the documentation of the (S)FTP interfaces.

[zooko]

Do we need to document this problem with mutable files in FTP? I don't think it is serious enough to deserve an entry in docs/known_issues.txt, but maybe in docs/frontends/FTP-and-SFTP.txt? Oh, I see there is already a note in there:

Mutable files are not supported by the FTP frontend.

(from docs/frontends/FTP-and-SFTP.txt@4439#L201) Hm, this documentation seems incomplete. What happens if I put a mutable file into a directory with tahoe put or tahoe ln or the wui, and then I list that directory using my FTP client?

[davidsarah]

Replying to zooko:

Do we need to document this problem with mutable files in FTP? I don't think it is serious enough to deserve an entry in docs/known_issues.txt, but maybe in docs/frontends/FTP-and-SFTP.txt? Oh, I see there is already a note in there:

Mutable files are not supported by the FTP frontend.

(from docs/frontends/FTP-and-SFTP.txt@4439#L201) Hm, this documentation seems incomplete. What happens if I put a mutable file into a directory with tahoe put or tahoe ln or the wui, and then I list that directory using my FTP client?

Well, according to the bug description it hangs (it's not clear whether the client hangs or the FTP server, probably the former).

The patch has not been applied because of the problem in comment:3. On the other hand, hanging on a directory listing is a more serious problem than failing to download the file correctly in some (broken) clients, so perhaps the patch should be applied anyway. Not for 1.7.0, though.

[zooko]

This would be appropriate for 1.7.1 with the addition of a NEWS snippet and a patch to docs/frontends/FTP-and-SFTP.txt .

[davidsarah]

Out of time for 1.7.1.

[davidsarah]

Out of time for 1.8.

This ticket does not affect SFTP; that was fixed for mutable files in 1.7.0.

[davidsarah]

Note that in e05c6c2c7d25db66, docs/frontends/FTP-and-SFTP.txt was changed to say:

Mutable files are not supported by the FTP frontend (ticket #680). Currently, a directory containing mutable files cannot even be listed over FTP.

[zooko]

Well if I understand comment:14 correctly, this ticket merely needs NEWS and docs and can be merged for v1.9.0. If I understand correctly, this match makes it so that mutable files get a 0 size instead of a '?' size. A 0 size makes certain FTP clients (I don't know which) fail to download the mutable file. A '?' size makes certain other ones (I don't know which) hang when listing the directory containing the file. I'm not 100% sure that the latter kind hang when listing the directory containing the file, or hang when trying to download the file.

So I believe the next step is to update NEWS and FTP-and-SFTP.rst​.

Also we should commit it to trunk early in the v1.9.0 process and ask people to try it out with their favorite FTP client.

[davidsarah]

Replying to zooko:

So I believe the next step is to update NEWS and FTP-and-SFTP.rst​.

+1. I'll do that.

[davidsarah]

Because we'll probably have a fairly long test cycle for 1.9, it seems like a good time to apply the fix to give the size as zero, and do interoperability testing with that.

I'll rebase the patch for trunk if needed.

[davidsarah]

This missed the 1.9 deadline.

[davidsarah]

​https://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/1688/fix_ftpd_size_type.dpatch has been committed (in 7f6ee7e9180377bc) and makes a change equivalent to ftp.patch​.

This doesn't necessarily make mutable files work with FTP. We should test interoperability with several FTP clients.

[davidsarah]

Oh, the reason it fails is that ReadFile in src/allmydata/frontends/ftpd.py calls the read method of a filenode, which only exists for immutable filenodes. (It also exists on MutableFileVersion, but not MutableFileNode.)

[davidsarah]

Replying to davidsarah:

Oh, the reason it fails is that ReadFile in src/allmydata/frontends/ftpd.py calls the read method of a filenode, which only exists for immutable filenodes.

To fix that, do something like:

    def send(self, consumer):
        d = self.node.get_best_readable_version()
        d.addCallback(lambda v: v.read(consumer))
        return d # when consumed

in ReadFile. But we'd also need tests for FTP (#512).

[davidsarah]

I think we ended up being deeply confused on this ticket by warner's speculation in comment:5 :-)