tahoe-lafs/trac

review Brian's patches for #607 #830

New issue

Closed

opened 2009-11-13 03:09:09 +00:00 by zooko · 24 comments

zooko commented

2009-11-13 03:09:09 +00:00

per http://allmydata.org/pipermail/tahoe-dev/2009-October/003061.html

per <http://allmydata.org/pipermail/tahoe-dev/2009-October/003061.html>

zooko added the

labels 2009-11-13 03:09:09 +00:00

zooko added this to the 1.6.0 milestone 2009-11-13 03:09:09 +00:00

zooko self-assigned this 2009-11-13 03:09:09 +00:00

zooko commented

2009-11-13 04:11:28 +00:00

Author

Okay here is the timeline of all patches committed since the 1.5.0 release until today: http://allmydata.org/trac/tahoe/timeline?from=2009-11-13&daysback=104&changeset=on&update=Update

First up: changeset:0d5dc5161791a90a: "Overhaul IFilesystemNode handling, to simplify tests and use POLA internally."

Okay here is the timeline of all patches committed since the 1.5.0 release until today: <http://allmydata.org/trac/tahoe/timeline?from=2009-11-13&daysback=104&changeset=on&update=Update> First up: changeset:0d5dc5161791a90a: "Overhaul IFilesystemNode handling, to simplify tests and use POLA internally."

zooko commented

2009-12-29 00:14:02 +00:00

Author

I feel uncomfortable about the prospect that we might release a new stable version of Tahoe-LAFS without anybody having reviewed these deep changes. I'm marking this as "critical" to remind myself that I'm currently uncomfortable releasing a new stable release without first closing this ticket. I still own this ticket because I plan to do it, but if anyone else wants to do it I would definitely appreciate the help. We could even split up the work.

zooko added

p/critical

and removed

p/major

labels 2009-12-29 00:14:02 +00:00

warner commented

2010-01-09 02:29:38 +00:00

I'd offer to help with the review, but, well, reviewing one's own patches might cause a recursive loop in the universe and destroy us all :).

But seriously, if there's anything I can explain or provide more background on, just give a yell.

I'd offer to help with the review, but, well, reviewing one's own patches might cause a recursive loop in the universe and destroy us all :). But seriously, if there's anything I can explain or provide more background on, just give a yell.

francois commented

2010-01-10 01:42:09 +00:00

Owner

I made a functional review of these new immutable dirnodes and their
use by tahoe backup.

I've been running tahoe backup on a daily basis to save my precious
data for about 8 months now. After an upgrade to the latest trunk, I ran
a backup manually which, as expected, created a new DIR-IMM dirnode
inside Archive and a link to it named Latest.

Here are my observations so far:

An older Tahoe gateway did not recognized the new DIR-IMM

immutable directory and correctly showed an question mark on the webapi.

It seems to break the blackmatch FUSE implementation, an exception is

raised when it tries to access an immutable dirnode.

Sat Jan  9 22:25:05 2010: exception:
Traceback (most recent call last):
  File "/home/francois/WORK/dev/tahoe/contrib/fuse/impl_c/blackmatch.py", line 177, in inner_logexc
    ret = meth(self, *args, **kwargs)
  File "/home/francois/WORK/dev/tahoe/contrib/fuse/impl_c/blackmatch.py", line 314, in done
    precondition(not self.read_heap, self.read_heap, self.target_size, self.get_size())
  File "/usr/local/lib/python2.6/dist-packages/allmydata_tahoe-1.5.0_r4054-py2.6.egg/allmydata/util/assertutil.py", line 46, in precondition
    raise AssertionError, "".join(msgbuf)
AssertionError: precondition: [(3832, 0, <Deferred at 0x3c94440>)] <type 'list'>, 264 <type 'int'>, 264 <type 'int'>

I'll run 'tahoe deep-check' to see if it works correctly.

I made a functional review of these new immutable dirnodes and their use by *tahoe backup*. I've been running *tahoe backup* on a daily basis to save my precious data for about 8 months now. After an upgrade to the latest trunk, I ran a backup manually which, as expected, created a new *DIR-IMM* dirnode inside *Archive* and a link to it named *Latest*. Here are my observations so far: 1. An older Tahoe gateway did not recognized the new *DIR-IMM* > immutable directory and correctly showed an question mark on the webapi. 2. It seems to break the blackmatch FUSE implementation, an exception is > raised when it tries to access an immutable dirnode. ``` Sat Jan 9 22:25:05 2010: exception: Traceback (most recent call last): File "/home/francois/WORK/dev/tahoe/contrib/fuse/impl_c/blackmatch.py", line 177, in inner_logexc ret = meth(self, *args, **kwargs) File "/home/francois/WORK/dev/tahoe/contrib/fuse/impl_c/blackmatch.py", line 314, in done precondition(not self.read_heap, self.read_heap, self.target_size, self.get_size()) File "/usr/local/lib/python2.6/dist-packages/allmydata_tahoe-1.5.0_r4054-py2.6.egg/allmydata/util/assertutil.py", line 46, in precondition raise AssertionError, "".join(msgbuf) AssertionError: precondition: [(3832, 0, <Deferred at 0x3c94440>)] <type 'list'>, 264 <type 'int'>, 264 <type 'int'> ``` I'll run 'tahoe deep-check' to see if it works correctly.

francois commented

2010-01-10 17:12:18 +00:00

Owner

My daily tahoe backup got a huge performance hit after the upgrade from 1.5.0 to trunk. This the result of a typical daily run on 1.5.0 (+ a few patches).

 16 files uploaded (186918 reused), 67 directories created (26010 reused), 0 problematic files skipped
 backup done, elapsed time: 1:03:32

This is the result of the first run after upgrade.

 1056 files uploaded (201639 reused), 14107 directories created (19134 reused)
 backup done, elapsed time: 9:46:20

I've just launched a second run to determine whether it's only a one-time penalty due to the upgrade or if it's another problem. If this behavior is normal, it's probably worth begin mentioned in the release notes.

My daily *tahoe backup* got a huge performance hit after the upgrade from 1.5.0 to trunk. This the result of a typical daily run on 1.5.0 (+ a few patches). ``` 16 files uploaded (186918 reused), 67 directories created (26010 reused), 0 problematic files skipped backup done, elapsed time: 1:03:32 ``` This is the result of the first run after upgrade. ``` 1056 files uploaded (201639 reused), 14107 directories created (19134 reused) backup done, elapsed time: 9:46:20 ``` I've just launched a second run to determine whether it's only a one-time penalty due to the upgrade or if it's another problem. If this behavior is normal, it's probably worth begin mentioned in the release notes.

warner commented

2010-01-10 21:30:44 +00:00

If you keep seeing slowdowns and can confirm that nothing much has changed between subsequent runs, let's take a look at the SQL schema and make sure we're not missing an index on something important.

I'll add a note to the release notes to prepare folks for this change and for the initial slowdown.

Yeah, the first run with the new immutable-directory code will have to re-upload all of your directories, so you should see a slowdown on the first run. But after that, the second and later runs should be super quick, much faster than the second-and-later runs of the old (mutable directory) code, because the immutable-directory code will be doing fast sqlite lookups instead of a bunch of tahoe-side directory reads. If you keep seeing slowdowns and can confirm that nothing much has changed between subsequent runs, let's take a look at the SQL schema and make sure we're not missing an index on something important. I'll add a note to the release notes to prepare folks for this change and for the initial slowdown.

francois commented

2010-01-10 22:00:03 +00:00

Owner

Yes! The subsequent runs are way faster.

 0 files uploaded (197702 reused), 0 directories created (26372 reused)
 535 files checked, 0 directories checked
 backup done, elapsed time: 0:05:30

Yes! The subsequent runs are way faster. ``` 0 files uploaded (197702 reused), 0 directories created (26372 reused) 535 files checked, 0 directories checked backup done, elapsed time: 0:05:30 ```

zooko commented

2010-01-11 00:20:38 +00:00

Author

I opened #894 (blackmatch fuse doesn't know what to think about immutable directories).

kevan commented

2010-01-12 18:47:54 +00:00

Owner

I read this comment:

Call this to override the size of the RSA keys created for new
mutable files. The default of None means to let mutable.filenode
choose its own size, which means 2048 bits.

for KeyGenerator.set_default_keysize to mean that if I call that method with None as an argument, the key generator will know to generate keys of 2048 bits. If I understand the code correctly, what actually happens is that the default keysize (which is in fact 2048 bits before I call this method) is overwritten with None.

If I later call KeyGenerator.generate without specifying a keysize, None is passed (depending on whether a remote key generator is running) to the remote key generator (in source:src/allmydata/key_generator.py) or to the RSA implementation in pycryptopp. In neither case of these cases, from what I understand, is the comment actually true -- in the former, the key generator will prefer the passed key size to its default (which is also 2048 bits) and pass it to pycryptopp, and in the latter the configured keysize will be passed to pycryptopp, which will in both cases complain about its arguments.

I read this comment: ``` Call this to override the size of the RSA keys created for new mutable files. The default of None means to let mutable.filenode choose its own size, which means 2048 bits. ``` for [KeyGenerator](wiki/KeyGenerator).set_default_keysize to mean that if I call that method with None as an argument, the key generator will know to generate keys of 2048 bits. If I understand the code correctly, what actually happens is that the default keysize (which is in fact 2048 bits before I call this method) is overwritten with None. If I later call [KeyGenerator](wiki/KeyGenerator).generate without specifying a keysize, None is passed (depending on whether a remote key generator is running) to the remote key generator (in source:src/allmydata/key_generator.py) or to the RSA implementation in pycryptopp. In neither case of these cases, from what I understand, is the comment actually true -- in the former, the key generator will prefer the passed key size to its default (which is also 2048 bits) and pass it to pycryptopp, and in the latter the configured keysize will be passed to pycryptopp, which will in both cases complain about its arguments.

kevan commented

2010-01-12 18:50:30 +00:00

Owner

Also note that KeyGenerator in client.py has the same name as the key generator in source:src/allmydata/key_generator.py. I'm not sure if that's a stylistic concern or anything -- they're probably unlikely to ever be in scope in the same context.

Also note that [KeyGenerator](wiki/KeyGenerator) in client.py has the same name as the key generator in source:src/allmydata/key_generator.py. I'm not sure if that's a stylistic concern or anything -- they're probably unlikely to ever be in scope in the same context.

kevan commented

2010-01-12 18:56:49 +00:00

Owner

erm, nevermind -- I read that comment again and realized that I didn't read it right -- mutable.filenode is not the same thing as client.KeyGenerator.

zooko commented

2010-01-12 20:00:06 +00:00

Author

Please post on this ticket which specific patches you have reviewed so I can skip those ones when I'm looking for Brian-patches to review.

Thank you very much for reviewing Brian's patches! I desperately need help with this task. (And I'm prioritizing reviewing your patches for #778, which I have been doing on the bus to work every morning by the way.) Please post on this ticket which specific patches you have reviewed so I can skip those ones when I'm looking for Brian-patches to review.

warner commented

2010-01-13 00:48:37 +00:00

Kevan: yeah, allmydata.client.KeyGenerator is what mutable-file creation uses to get an RSA keypair, and it either generates one locally or sends a message to a remote key-generator process (defined in allmydata.key_generator.KeyGenerator). I agree that the comment needs improvement. I touched that code to make it easier for most unit tests to generate small/fast keys, while still allowing at least one test to use full-sized 2048-bit keys, and of course to make sure that normal operations use full-size keys. As you observed, each call to KeyGenerator.generate() gets to pick a keysize, and if the caller doesn't provide one, it will use self.default_keysize which can be controlled by a call to KeyGenerator.set_default_keysize(), and defaults to 2048.

I think the comment on set_default_keysize was moved from generate without sufficient editing. I've just pushed a patch changeset:fb879ddea40c03e7 to improve the comments a bit.. let me know if you see any mistakes in the new text.

Kevan: yeah, `allmydata.client.KeyGenerator` is what mutable-file creation uses to get an RSA keypair, and it either generates one locally or sends a message to a remote key-generator process (defined in `allmydata.key_generator.KeyGenerator`). I agree that the comment needs improvement. I touched that code to make it easier for most unit tests to generate small/fast keys, while still allowing at least one test to use full-sized 2048-bit keys, and of course to make sure that normal operations use full-size keys. As you observed, each call to `KeyGenerator.generate()` gets to pick a keysize, and if the caller doesn't provide one, it will use `self.default_keysize` which can be controlled by a call to `KeyGenerator.set_default_keysize()`, and defaults to 2048. I think the comment on `set_default_keysize` was moved from `generate` without sufficient editing. I've just pushed a patch changeset:fb879ddea40c03e7 to improve the comments a bit.. let me know if you see any mistakes in the new text.

kevan commented

2010-01-14 00:53:42 +00:00

Owner

zooko: I've looked over the functional changes for changeset:0d5dc5161791a90a, but none of the tests.

warner: Yes, that's much better. Thanks for indulging me. :)

zooko: I've looked over the functional changes for changeset:0d5dc5161791a90a, but none of the tests. warner: Yes, that's much better. Thanks for indulging me. :)

kevan commented

2010-01-15 03:29:56 +00:00

Owner

I've just finished looking over the tests.

You store the nodemaker as self.nodemaker in [test_dirnode.Dirnode2]source:src/allmydata/test/test_dirnode.py?rev=4045#L780, and you've switched the tests to use nodemaker where they used client before, but you've left self.client as an ivar. Unless I'm missing something, none of the tests use it, so it should work just fine as a local variable in setUp.

Other than that, everything looks fine. Though there isn't a set of tests specifically for the NodeMaker (unless I've missed them), the fact that it is an abstraction of existing (and, from what I can tell, well-tested) behavior rather than new behavior means that this is probably a non-issue.

I've now looked at all of changeset:0d5dc5161791a90a.

I've just finished looking over the tests. You store the nodemaker as `self.nodemaker` in [test_dirnode.Dirnode2]source:src/allmydata/test/test_dirnode.py?rev=4045#L780, and you've switched the tests to use nodemaker where they used client before, but you've left `self.client` as an ivar. Unless I'm missing something, none of the tests use it, so it should work just fine as a local variable in `setUp`. Other than that, everything looks fine. Though there isn't a set of tests specifically for the [NodeMaker](wiki/NodeMaker) (unless I've missed them), the fact that it is an abstraction of existing (and, from what I can tell, well-tested) behavior rather than new behavior means that this is probably a non-issue. I've now looked at all of changeset:0d5dc5161791a90a.

warner commented

2010-01-15 06:36:04 +00:00

Yeah, you're right. I've pushed changeset:9ab7524f0da3fcf8 to implement your suggestion.

And yes, there's no test_nodemaker, but all of nodemaker.py was extracted from client.py, and during the development process I iterated to attain full line-coverage on nodemaker.py . So I wasn't too worried about it. Plus I couldn't think of a good way to test it in isolation.

A lot of changeset:0d5dc5161791a90a was changing everything else to stop needing a Client, and instead using a Nodemaker or some other set of objects. And those changes should be covered by the existing tests for all of those "elses". Plus changing a lot of tests themselves to be smaller and not use a full Client when possible.

Yeah, you're right. I've pushed changeset:9ab7524f0da3fcf8 to implement your suggestion. And yes, there's no test_nodemaker, but all of nodemaker.py was extracted from client.py, and during the development process I iterated to attain full line-coverage on nodemaker.py . So I wasn't too worried about it. Plus I couldn't think of a good way to test it in isolation. A lot of changeset:0d5dc5161791a90a was changing everything *else* to stop needing a Client, and instead using a Nodemaker or some other set of objects. And those changes should be covered by the existing tests for all of those "elses". Plus changing a lot of tests themselves to be smaller and not use a full Client when possible.

kevan commented

2010-01-19 04:34:48 +00:00

Owner

Is there a canonical list of changesets that need to be reviewed for this to be closed?

zooko commented

2010-01-19 04:44:24 +00:00

Author

That's a good question. How about if I kind of scout around for patches that Brian committed that seem to be relevant to #607... Oh boy there are a lot of them.

changeset:5fe713fc52dc331b, changeset:f85690697a21e669, changeset:3ee740628ab32aae, changeset:e2ffc3dc03df8d73, changeset:304aadd4f7632afe, changeset:f871c3bb3ddc3b42, changeset:d079eb45f6581c27, changeset:cf65cc2ae3cc1062, changeset:b30041c5ecf3e2b6, changeset:c2520e4ec76195fb, changeset:480e1d318dd46619, changeset:ea373de042c49ba1 (you might be especially interested in that one. :-)), changeset:8a7c980e3765f89c, changeset:b4ec86c95a64d911, changeset:1273b5c233b076aa, changeset:768c76aa5fbe2c7f, changeset:2695af91a73661bf, changeset:f47672d12acb9042, changeset:cc422f8dc00d5cd3, changeset:5fe713fc52dc331b, changeset:cc422f8dc00d5cd3, changeset:512fe3ad62d0ad94, changeset:f85690697a21e669, changeset:834b20210ac37194.

Phewf! That is quite a lot, but the more of it you can review before the Tahoe-LAFS v1.6 release the better. Even if you don't find any major bugs, you will definitely be gaining expertise in the core Tahoe-LAFS logic -- expertise which I hope you will go on to use in Tahoe-LAFS v1.7. :-)

That's a good question. How about if I kind of scout around for patches that Brian committed that seem to be relevant to #607... Oh boy there are a lot of them. changeset:5fe713fc52dc331b, changeset:f85690697a21e669, changeset:3ee740628ab32aae, changeset:e2ffc3dc03df8d73, changeset:304aadd4f7632afe, changeset:f871c3bb3ddc3b42, changeset:d079eb45f6581c27, changeset:cf65cc2ae3cc1062, changeset:b30041c5ecf3e2b6, changeset:c2520e4ec76195fb, changeset:480e1d318dd46619, changeset:ea373de042c49ba1 (you might be especially interested in that one. :-)), changeset:8a7c980e3765f89c, changeset:b4ec86c95a64d911, changeset:1273b5c233b076aa, changeset:768c76aa5fbe2c7f, changeset:2695af91a73661bf, changeset:f47672d12acb9042, changeset:cc422f8dc00d5cd3, changeset:5fe713fc52dc331b, changeset:cc422f8dc00d5cd3, changeset:512fe3ad62d0ad94, changeset:f85690697a21e669, changeset:834b20210ac37194. Phewf! That is quite a lot, but the more of it you can review before the Tahoe-LAFS v1.6 release the better. Even if you don't find any major bugs, you will definitely be gaining expertise in the core Tahoe-LAFS logic -- expertise which I hope you will go on to use in Tahoe-LAFS v1.7. :-)

kevan commented

2010-01-19 04:49:12 +00:00

Owner

Excellent -- thanks for that. I'm mostly reviewing these when I have a spare moment during my day, so I'm not sure how much I'll get done, but I'll try.

Hm. Well, let's say for now that I'll take care of changeset:5fe713fc52dc331b. If I finish that, I'll start on the next changeset that is unclaimed.

Excellent -- thanks for that. I'm mostly reviewing these when I have a spare moment during my day, so I'm not sure how much I'll get done, but I'll try. Hm. Well, let's say for now that I'll take care of changeset:5fe713fc52dc331b. If I finish that, I'll start on the next changeset that is unclaimed.

daira commented

2010-01-24 21:57:34 +00:00

When doing #833, I looked over all of the dirnode and web tests for immutable directories, and also the webapi.txt documentation. Those all looked OK; anything I found is addressed in the #833 patch.

zooko commented

2010-01-28 15:04:04 +00:00

Author

Okay, good enough! Thanks, Kevan and David-Sarah!

zooko added the

r/fixed

label 2010-01-28 15:04:04 +00:00

zooko closed this issue

2010-01-28 15:04:04 +00:00

kevan commented

2010-02-04 00:51:33 +00:00

Owner

I just finished looking through changeset:5fe713fc52dc331b, and didn't see any problems. I'll probably poke through some of the rest of these eventually, so I guess I'll reopen this if I find any problems.

kevan commented

2010-02-04 04:49:51 +00:00

Owner

Okay, from changeset:f85690697a21e669:

Am I missing something, or could you remove the self.convergence ivar from Client? From what I can tell, the only place it is used now is to make the secret holder that the nodemaker uses to get the convergence secret.

Well, I guess we can't remove it: we have web/unlinked.py,
web/filenode.py, frontends/ftpd.py and frontends/sftpd.py
that all reference that part of Client to get the convergence secret. It seems
like it would be more elegant if there was only
one canonical place for that -- the SecretHolder instance. We deal with this challenge for other sorts of files with methods in Client -- see

if mutable:
            req.content.seek(0)
            data = req.content.read()
            d = client.create_mutable_file(data)
            def _uploaded(newnode):
                d2 = self.parentnode.set_node(self.name, newnode,
                                              overwrite=replace)
                d2.addCallback(lambda res: newnode)
                return d2
            d.addCallback(_uploaded)
        else:
            uploadable = FileHandle(req.content, convergence=client.convergence)
            d = self.parentnode.add_file(self.name, uploadable,
                                         overwrite=replace)

where there is a method in client that talks to the nodemaker and makes a new mutable file, abstracting away the details of convergence and so on, but none for an immutable file. But then there isn't a method to make an immutable file in nodemaker, either, so there's not a lot to use as an abstraction. I guess that's kind of rambly, but that's what I was thinking when reading that changeset -- why not add immutable file logic to the nodemaker, and use that to eliminate external dependencies on things that aren't the SecretHolder?

The rest of the changes look okay.

Okay, from changeset:f85690697a21e669: Am I missing something, or could you remove the `self.convergence` ivar from `Client`? From what I can tell, the only place it is used now is to make the secret holder that the nodemaker uses to get the convergence secret. Well, I guess we can't remove it: we have `web/unlinked.py`, `web/filenode.py`, `frontends/ftpd.py` and `frontends/sftpd.py` that all reference that part of `Client` to get the convergence secret. It seems like it would be more elegant if there was only one canonical place for that -- the `SecretHolder` instance. We deal with this challenge for other sorts of files with methods in `Client` -- see ``` if mutable: req.content.seek(0) data = req.content.read() d = client.create_mutable_file(data) def _uploaded(newnode): d2 = self.parentnode.set_node(self.name, newnode, overwrite=replace) d2.addCallback(lambda res: newnode) return d2 d.addCallback(_uploaded) else: uploadable = FileHandle(req.content, convergence=client.convergence) d = self.parentnode.add_file(self.name, uploadable, overwrite=replace) ``` where there is a method in client that talks to the nodemaker and makes a new mutable file, abstracting away the details of convergence and so on, but none for an immutable file. But then there isn't a method to make an immutable file in nodemaker, either, so there's not a lot to use as an abstraction. I guess that's kind of rambly, but that's what I was thinking when reading that changeset -- why not add immutable file logic to the nodemaker, and use that to eliminate external dependencies on things that aren't the `SecretHolder`? The rest of the changes look okay.

warner commented

2010-02-05 04:29:34 +00:00

Replying to kevan:

Well, I guess we can't remove it: we have web/unlinked.py,
web/filenode.py, frontends/ftpd.py and frontends/sftpd.py
that all reference that part of Client to get the convergence secret.
It seems like it would be more elegant if there was only one canonical
place for that -- the SecretHolder instance.

Yeah.. I'm not sure I was thinking about it at the time, but some day we're
likely to add a webapi facility to let clients provide the convergence secret
on an upload-by-upload basis, which is a vague argument for continuing to let
at least web/*.py grab the default from Client and then pass it
into the uploader code themselves.

There will be other values like this in the future.. I'm thinking of the
Accounting authority string here (for which there might be a client-wide
default, or there may be no default and webapi callers are obligated to
provide their own on each request).

We deal with this challenge for other sorts of files with methods in
Client

Yeah, I've updated more of the mutable-using code than the immutable code so
far (the immutable code is among the oldest in the tree).

So far, NodeMaker has acquired responsibility for turning caps into
nodes, and then picked up methods to create brand new nodes:
create_mutable_file, create_new_mutable_directory, and most
recently create_immutable_directory. So the next logical step would be
to give it a way to create new immutable filenodes,
create_immutable_file which would be known as upload() in the
vernacular.

(oh, it's also worth pointing out that mutable files don't use convergence..
wouldn't even make sense with them, so that's one fewer arguments needed for
client.create_mutable_file())

It would probably be a good start to get rid of dirnode.add_file,
change the signature of set_node to return the node that was just
added, and then replace the example you cite with something like:

if mutable:
    req.content.seek(0)
    data = req.content.read()
    d = client.create_mutable_file(data)
else:
    uploadable = FileHandle(req.content, convergence=client.convergence)
    d = client.upload(uploadable)
d.addCallback(lambda newnode: 
              self.parentnode.set_node(self.name, newnode, overwrite=replace)

If the nodemaker learned how to upload stuff, that could probably turn into:

if mutable:
    req.content.seek(0)
    data = req.content.read()
    d = nodemaker.create_mutable_file(data)
else:
    uploadable = FileHandle(req.content) # convergence?
    d = nodemaker.create_immutable_file(uploadable)
d.addCallback(lambda newnode: 
              self.parentnode.set_node(self.name, newnode, overwrite=replace)

As I get more comfortable with the Producer/Consumer framework, I see some
places where I can get rid of the funky custom classes that I built (like
FileHandle and IUploadable) and replace them with more normal
things like file-like objects and IProducers. I'm not yet sure I could
do away with IUploadable, but it's worth exploring. If both
create_mutable_file and create_immutable_file were defined to
accept either a string, a Producer, or a file-like object, then that code
could turn into:

if mutable:
    d = nodemaker.create_mutable_file(req.content)
else:
    d = nodemaker.create_immutable_file(req.content) # convergence?
d.addCallback(lambda newnode: 
              self.parentnode.set_node(self.name, newnode, overwrite=replace)

Replying to [kevan](/tahoe-lafs/trac/issues/830#issuecomment-374330): > > Well, I guess we can't remove it: we have `web/unlinked.py`, > `web/filenode.py`, `frontends/ftpd.py` and `frontends/sftpd.py` > that all reference that part of `Client` to get the convergence secret. > It seems like it would be more elegant if there was only one canonical > place for that -- the `SecretHolder` instance. Yeah.. I'm not sure I was thinking about it at the time, but some day we're likely to add a webapi facility to let clients provide the convergence secret on an upload-by-upload basis, which is a vague argument for continuing to let at least `web/*.py` grab the default from `Client` and then pass it into the uploader code themselves. There will be other values like this in the future.. I'm thinking of the Accounting authority string here (for which there might be a client-wide default, or there may be no default and webapi callers are obligated to provide their own on each request). > We deal with this challenge for other sorts of files with methods in > `Client` Yeah, I've updated more of the mutable-using code than the immutable code so far (the immutable code is among the oldest in the tree). So far, `NodeMaker` has acquired responsibility for turning caps into nodes, and then picked up methods to create brand new nodes: `create_mutable_file`, `create_new_mutable_directory`, and most recently `create_immutable_directory`. So the next logical step would be to give it a way to create new immutable filenodes, `create_immutable_file` which would be known as `upload()` in the vernacular. (oh, it's also worth pointing out that mutable files don't use convergence.. wouldn't even make sense with them, so that's one fewer arguments needed for `client.create_mutable_file()`) It would probably be a good start to get rid of `dirnode.add_file`, change the signature of `set_node` to return the node that was just added, and then replace the example you cite with something like: ``` if mutable: req.content.seek(0) data = req.content.read() d = client.create_mutable_file(data) else: uploadable = FileHandle(req.content, convergence=client.convergence) d = client.upload(uploadable) d.addCallback(lambda newnode: self.parentnode.set_node(self.name, newnode, overwrite=replace) ``` If the nodemaker learned how to upload stuff, that could probably turn into: ``` if mutable: req.content.seek(0) data = req.content.read() d = nodemaker.create_mutable_file(data) else: uploadable = FileHandle(req.content) # convergence? d = nodemaker.create_immutable_file(uploadable) d.addCallback(lambda newnode: self.parentnode.set_node(self.name, newnode, overwrite=replace) ``` As I get more comfortable with the Producer/Consumer framework, I see some places where I can get rid of the funky custom classes that I built (like `FileHandle` and `IUploadable`) and replace them with more normal things like file-like objects and `IProducer`s. I'm not yet sure I could do away with `IUploadable`, but it's worth exploring. If both `create_mutable_file` and `create_immutable_file` were defined to accept either a string, a Producer, or a file-like object, then that code could turn into: ``` if mutable: d = nodemaker.create_mutable_file(req.content) else: d = nodemaker.create_immutable_file(req.content) # convergence? d.addCallback(lambda newnode: self.parentnode.set_node(self.name, newnode, overwrite=replace) ```