refactor download interfaces to treat immutable files and mutable versions more uniformly

[davidsarah]

Currently, downloading a mutable file is quite complicated unless the download_best_version() method of IMutableFileNode is used. This has several disadvantages:

• client code has to fork depending on whether it is downloading a mutable or immutable file.
• download_best_version() returns a Deferred for the whole contents, it does not allow directing the download to a consumer.
• there is the potential for race conditions when the client wants to get several pieces of information about the same version.

The attachment gives some proposed interface changes worked out with Brian on IRC. Among other improvements these would allow calling get_best_readable_version() on any filenode, and then using read to download to a consumer. For the time being mutable files would still be downloaded in a single lump, but clients using these new interfaces would not have to change when we fix that.

('diff' got confused and started interleaving changes to unrelated methods, so compare the attachment with src/allmydata/interfaces.py​.)

[davidsarah]

Proposed refactoring of file download interfaces

[davidsarah]

Too disruptive / not enough time left to do this for 1.7.

[zooko]

Brian: is your new-downloader patch predicated on this patch? If so then we either need to bite the bullet and accept this disruptive refactoring for 1.7, at the risk of either destabilizing or delaying 1.7 release, or else plan to release 1.7 without new-downloader.

(David-Sarah and Brian and I discussed the new-downloader in v1.7 or not on IRC. We agreed that it might be good to plan v1.7 with no new-downloader and that it might be good to delay v1.7 by a few weeks to have a solid, reliable new-downloader.)

[davidsarah]

Note that the patch attached to this ticket is only a proposed API, not an implementation of it.

[davidsarah]

Here​ is an example of code that forks depending on whether it's downloading a mutable or immutable file, in the SFTP frontend:

    # TODO: use download interface described in #993 when implemented.
    if filenode.is_mutable():
        self.async.addCallback(lambda ign: filenode.download_best_version())
        def _downloaded(data):
            self.consumer = OverwriteableFileConsumer(len(data), tempfile_maker)
            self.consumer.write(data)
            self.consumer.finish()
            return None
        self.async.addCallback(_downloaded)
    else:
        download_size = filenode.get_size()
        assert download_size is not None, "download_size is None"
        self.consumer = OverwriteableFileConsumer(download_size, tempfile_maker)
        def _read(ign):
            if noisy: self.log("_read immutable", level=NOISY)
            filenode.read(self.consumer, 0, None)
        self.async.addCallback(_read)

With the proposed interface it could be:

    self.async.addCallback(lambda ign: filenode.get_best_version())
    def _got_best_version(version):
        if noisy: self.log("_got_best_version(%r)" % (version,), level=NOISY)
        download_size = version.get_size()
        assert download_size is not None, "download_size is None"
        self.consumer = OverwriteableFileConsumer(download_size, tempfile_maker)
        version.read(self.consumer, 0, None)
    self.async.addCallback(_got_best_version)

Since the new downloader is milestone 1.8, this can wait until after 1.7.1, though.

[davidsarah]

Kevan is working on this in the patches for #393 (MDMF). Some comments on those patches relating to this ticket:

class IMutableFileVersion(IReadable):
    # TODO: Can this be overwrite instead of replace?
    def replace(new_contents):

Yes, it can be, and should take an IMutableUploadable rather than a bytestring.

class IReadable(Interface):
    def is_readonly():
        """Return True if this reference provides mutable access to the given
        file or directory (i.e. if you can modify it), or False if not. Note
        that even if this reference is read-only, someone else may hold a
        read-write reference to it.

        For an IReadable returned by get_best_readable_version(), this will
        always return True, but for instances of subinterfaces such as
        IMutableFileVersion, it may return False."""

The second paragraph here could be misread; perhaps change it to

        For an IReadable returned by get_best_readable_version(), this will
        always return True. For instances of IMutableFileVersion (a subinterface
        of IReadable) that are returned by get_best_mutable_version(), it can
        return True or False.

[zooko]

How can we move this ticket out of review-needed state? I know! Kevan could review it and we could apply the patch to trunk! Kevan: interested? If we're not going to "apply the patch to trunk" because it is only interface changes and not implementation then let's remove the review-needed tag until we have an implementation.

[kevan]

An implementation of this ticket (including the interface) is in #393, so I don't think there's anything to apply here. I'll remove review-needed. I'm tempted to say that #993 could be marked as a duplicate of #393, but it might be useful to be able to discuss the interface independently of #393 as a whole, which is fairly long and involves other issues.

(Maybe we should make a trac keyword that says that a ticket is in particular need of design feedback or review but doesn't involve any code changes. Then 'review-needed' could be used only for code changes needing a review before they can be committed, and the other keyword could be used for tickets like #993, which don't have a patch that we'd want to apply but could benefit from other perspectives.)

[kevan]

This is still addressed by #393, so it should stay in the 1.9.0 milestone.

[davidsarah]

In ticket:413#comment:5, zooko said:

"roothash" means the root of the Merkle Tree over the (encrypted) shares of one specific version of the file, so it is sufficient to uniquely identify one version. Yes, everything that talks about "versions" of mutable files should specify the version as a (sequence_number, roothash) tuple.

This affects the new IMutableFileversion interface in interfaces.py: it should either grow a get_roothash() method, or get_sequence_number() should be changed to get_sequence_number_and_roothash() (or some more concise name) that returns a pair.

(The object represents a particular version, so a single method returning a pair is not necessary to avoid race conditions.)

[warner]

not making it into 1.9

[davidsarah]

The main changes suggested by this ticket were part of #393, and are present in 1.9. (comment:14 isn't implemented, but that isn't critical.)

[davidsarah]

Don't know why the Version field changed.

[warner]

does that mean we can close this?

[warner]

davidsarah said yes. Yay!