refactor: Adjustments to Stores & Agents

[matheus23]

Ongoing discussion: https://talk.fission.codes/t/rs-ucan-1-0-api-notes/5347/5

• Made delegation::Store::get return Option
• Make delegation::Store::get return Arcs
• Make delegation::Store::insert take &self instead of &mut self
• Similar changes for invocation::Store

[expede]

Make delegation::Store::get return Arcs

I missed a step on this

    fn get(
        &self,
        cid: Cid,
    ) -> Result<Option<Arc<Invocation<T, DID, V, C>>>, Self::InvocationStoreError>;

Invocations are immutable — won't that make them threadsafe? Why do they need to be Arced?

UPDTAE:
Or is this "just" a Rust internal interface bookkeeping thing?

See comment lower in the thread

[expede]

Also while we're talking about it: there's two approaches to the type signature. The nested version is what I have here...

// Current
get(/* ... */) -> Result<Option<Invocation<T, DID, V, C>>, Self::InvocationStoreError>;
//                       ^^^^^^
//          `Ok` branch include non-happy path

...where Self::InvocationStoreError is user-defined, so that Option separates out that there was no database error, but there's more unwrapping to do (ok -> some -> value). The other can also use nesting on the Err branch:

struct StoreErr<T> {
  NotFound(Cid),
  DbErr(T)
}

get(/* ... */) -> Result<Invocation<T, DID, V, C>>, StoreErr<Self::InvocationStoreError>>;
//                                                   ^^^^^^^^
//                                                     HERE

This cleans up the type a bit by putting anything not on the happy path over in the Err, but not having that Option is perhaps less idiomatic — thoughts?

[expede]

Invocations are immutable — won't that make them threadsafe?

Maybe to expose my mental model:

The Invocation is owned by the Store. This may be a database or an in-memory store. We can make as many clones of the Invocation as we want, because they never change. In my mind, the thing passed back from the store is either owned by the store. My understanding of the borrow system is that you can have [practically] infinite immutable refs.

I'm almost certainly missing a part of the picture, so mostly using this as a learning opportunity :)

[matheus23]

Invocations are immutable — won't that make them threadsafe? Why do they need to be Arced?

(EDIT: I reread the question a little later now, and I think I misunderstood it slightly. The below is still helpful, I think, but: Yes, immutable references are thread-safe. But it's not about thread-safety here, it's about not making the Store implementation be responsible for knowing when the Invocation should be freed.)

Requiring to return a &Invocation instead of an Invocation or Arc<Invocation> will make some implementations of invocation stores impossible - e.g. implementations backed by a database or by the file system.
The reason is that the reference means you're passing out a 'borrowed' invocation. But where are you borrowing it from? In the case of MemoryStore, you're borrowing it from the &self parameter, and self will "take care of" the Invocation's lifetime - it stores it itself, and takes care of when it needs to be freed.
But if you want to implement get via reading the filesystem, parsing an invocation, passing out a reference of it, and then return without storing the actual invocation itself anywhere. So where will it live? It would be freed at the end of the get method. But because you're also passing out a reference of it, rust forbids that kind of code due to possible use-after-frees.

With both Rc<Invocation> and Arc<Invocation>, the invocation is still immutable. I think of [A]Rc<T> as &T, but instead of figuring out when T needs to be dropped at compile time, it's figured out at runtime via reference counting.
Someone that has a reference to an Arc<Invocation> can't change other Arc<Invocation> instances. Or - another way of saying that is "Arc<T> doesn't provide interior mutability". Which is confusing since it appears in the Arc<Mutex and Arc<RwLock pattern a bunch, but it's the Mutex and RwLock that provide the interior mutability in those cases.

The reason I went for Arc over Rc is that in practice you want multiple thread to be able to have a reference to the same Invocation at the same time. With Rc the internal bookkeeping logic of that reference counter could have race conditions when used from multiple threads, so rust forbids its use in those circumstances.

They don't necessarily need to be Arced. You could also return owned instances of Invocation directly. But then that forces MemoryStore to clone them when passing them out (because the memory store wants to keep an instance for itself). With Arcs you get the best of both worlds: No lifetimes, and also cheap cloning. Not as cheap as passing around a reference, but still very good.

This cleans up the type a bit by putting anything not on the happy path over in the Err, but not having that Option is perhaps less idiomatic — thoughts?

I think the Option< version is totally fine.
The BlockStore trait at the moment returns an error on not found. It's been awkward in places where you expect the block to be missing sometimes, but a lot cleaner in many other places, where you expect every block to be resolvable.
So it kind of depends on that ratio? How often would you expect the caller to be "fine" with get not resolving?
Also - the associated type for errors is nice, but I want to point out that for simplicity, it's fine making it a concrete ucan::Error type (or perhaps a ucan::invocation::store::Error type, where one case is simply CustomError(Box<dyn StdError>) (or something along those lines, I forgot the exact incantation. Although I'm less sure of that last point.

[expede]

So where will it live?

Ah, right, this is the bit that I keep missing. I keep thinking of it as almost like an actor where there's some long-lived location representing "the database" that can hold various bits of memory, but that's not necessarily the case — I mean, you could build this, but that's way more work than an Arc<T>. Cool; thanks for explaining!

[expede]

A couple questions, oh wise Rustacean 🦀

Generality

I guess RwLock here because it's the most general option? Is this a typical interface for libraries (as opposed to applications)? It really makes me want HKTs 😛

Deadlocks

From the std docs:

The priority policy of the lock is dependent on the underlying operating system’s implementation, and this type does not guarantee that any particular policy will be used.
[...]
Potential deadlock example

That's... unsettling. Are there ways to guard against starvation other than "don't use a bad OS"?

[matheus23]

oh wise Rustacean 🦀

😂 🤦‍♂️ 😅

Deadlocks [...] That's... unsettling. Are there ways to guard against starvation other than "don't use a bad OS"?

It is unsettling! And you're wise in reading the documentation more than I have 😛
I chose RwLock because it should be faster than Mutex or equally fast in all cases, but admittedly, the deadlock potential on some OSes worries me, too. And performance shouldn't matter too much here, and if someone is worried about that, they could/should write a store implementation based on DashMap.
Should we change it to Mutex?

[matheus23]

Reading up a bit I think I had some misconceptions about RwLocks. They can be slower than Mutexes in some cases. Some people recommend "Use Mutex unless you know what you're doing".
And I clearly don't! So let's switch to mutex 👍

[matheus23]

By the way -

Is this a typical interface for libraries (as opposed to applications)?

I'm not sure I understand. The RwLock/Mutex is totally an implementation detail and shouldn't leak to the interface. It's a private field.
The MemoryStore/MemoryStoreInner is a typical way of structuring code.

[expede]

Should we account for a poisoned RwLock?

[matheus23]

I thought about this a bit. The poisoning is mostly important if you keep using a delegation store after it had panicked.
I don't think we have a reasonable way of restoring the invariants when it happens, or at least I don't think it's worth looking into that.
And given this is in the implementation of get_chain:

let all_powerlines = read_tx.index.get(&None).unwrap_or(&blank_map);
let all_aud_for_subject = read_tx.index.get(subject).unwrap_or(&blank_map);
let powerline_candidates = all_powerlines.get(aud).unwrap_or(&blank_set);
let sub_candidates = all_aud_for_subject.get(aud).unwrap_or(&blank_set);

Broken invariants at least wouldn't cause further panics.

[expede]

Perhaps worth making these pairs an option on the store?

[matheus23]

Maybe? I'm fine either way. Perhaps we just wait for a second case of this code happening, then we abstract?

[expede]

This is slightly ambiguous: is there a missing delegation in the prf field vs from the store. Have you considered something like CannotFindDelegation(Cid)?

[matheus23]

Yeah I like your suggestion!

[matheus23]

I took the freedom to adjust it to say DelegationNotFound ✌️

[expede]

Thanks again @matheus23! A couple questions for my own learning, but LGTM! 🚀