urbit / arvo

https://github.com/urbit/urbit
110 stars 57 forks source link

Clay cleanup #1175

Closed philipcmonk closed 5 years ago

philipcmonk commented 5 years ago

I'm a programmer today because a man at my church gave me a copy of Linux on a CD when I was a kid and told me to learn C. He also told me a story of when he was a kid. There was a small anthill where he was playing in his backyard. He tried destroying the anthill with a stick, but it kept coming back. Rather than figure out exactly how to remove this anthill surgically, he poured a flammable liquid (gasoline? I forget) in the anthill. He figured he'd pour until it was full, then light it. But it took a lot longer than he thought, and he poured in a lot of gasoline. Finally, he lit it, and he says it blew up much of his backyard. He didn't have ant problems for a long time.

This is the approach I've taken with Clay, both in #1169 and here. There was some bug in +wake, but that part of the code was the most impenetrable part of clay. The commit/merge flow was more fragile, but at least you could tell what it was doing. But +wake was a 200 line impenetrable, stateful function made worse by being almost duplicated in +start-request, except that one had some small but important differences. Rather that try to find the bug, I re-wrote both of these into a single pure function, +try-fill-sub, which is still long and somewhat complex, but it's stateless, drastically more readable than it was, and commented every few lines. As a bonus, the bug is gone. I don't know exactly what the bug was, but there were three or four I noticed and fixed as I was refactoring it.

All that to say, I was hoping this would be a small PR that showed a clean difference between the inside-out state machines of old clay for validating foreign updates and their monadic equivalents. But alas, it's a 1000 lines. Should be a lot easier to follow than the last PR, though. Most of the monadifying is in two commits. This is foreign simple requests and full updates: https://github.com/urbit/arvo/commit/80e22ab7c21e47dbbbb1ff1e9ec86d5fd895c749, and this is mounting a desk: https://github.com/urbit/arvo/commit/5a1cccdcea0e8fe3d8cec03049640c3b7bdaa7cb

In summary, this contains four changes:

This puts a bow on the changes I wanted to make to Clay in this pass, so I'll move on to Jael next. I'm much more confident in Clay than I was before, and I wouldn't be afraid to hack on it now. One change I would love is change the foreign update flow to only send the data we don't already have. Right now, it has a conservative heuristic for guessing whether the subscriber has the blobs referenced in the update, and it sends them if it doesn't know. But there's plenty of real-world situations where the subscribee doesn't know that the subscriber already has the blobs. It'd be nice to have an additional round-trip, so we first send just the hashes of the data, and the susbscriber asks for the data of whichever of the hashes it doesn't have yet.

philipcmonk commented 5 years ago

Thanks, fixed those three and also a bug where I was cancelling some ducts more than once when a ship sinks.

jtobin commented 5 years ago
      $d  ~|  %totally-temporary-error-please-replace-me  !!
      $p  ~|  %requesting-foreign-permissions-is-invalid  !!
      $t  ~|  %requesting-foreign-directory-is-vaporware  !!
      $u  ~|  %prolly-poor-idea-to-get-rang-over-network  !!
      $v  ~|  %weird-shouldnt-get-v-request-from-network  !!
      $z  ~|  %its-prolly-not-reasonable-to-request-ankh  !!

satisfying

belisarius222 commented 5 years ago

Now that the foreign-update code is monadified, will it work if we reload the vane in the middle of a foreign-update transaction? I'm wondering if there are any edge cases where we've received some Ames message that we won't receive again, and if we throw away our partially done foreign update after receiving the message, we'll lose the update forever.

philipcmonk commented 5 years ago

If you don't put in any special +load handling, this will currently just let the transaction complete (so you'll keep a pointer to the old kernel until it's done). You're right, though, that we should save the ames messages of the currently running transaction so that we can restart it if needed. This also applies to the foreign-request flow.

I guess the general principle is "if you need to be able to restart a transaction, you must save all the initial arguments to the transaction". In this case, for %info, %merg, and %mont we save the whole task. For foreign-update and foreign-request it's probably better to save [index=@ud (unit rand)] from deserializing the ames message. If you have a queue of requests, one way to implement this is to not remove from the queue until the transaction is completed.