sled: Tree::transaction API prevents error forwarding

For future reference, here’s a boiled down version of what I was trying to make work but cannot: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=82e38612f8e3d2a7a775cb0d897e143c

The reason it can’t work (thanks for pointing this out, @matklad) is because of the From bound on the error type, which blocks the default generic parameter from working here.

I’m going to close this because now aborts can pass arbitrary types through, but future issues might include ways of further cleaning this up. Thanks for the suggestions, everyone, and please let me know what you don’t like about that PR.

I don’t like all the new types and their long names, but I’m not sure if it makes the system harder or easier to use, so I could use some feedback if you have a chance to peek at it 😃

One reason I didn’t go with the conversion approach was because I wasn’t able to get type inference to respect default type parameters placed on a Transactional trait as a generic with a default of Error. This led to requiring an abort type to be specified on all transaction closures that did not abort, which I imagine would be a good chunk of them.

This is getting into tough API design territory, but my impression is that Fn may be a better fit than FnMut, though the code will compile with either. Let’s set aside retries, and consider a particular transaction that fails. If the closure running inside the transaction is a FnMut and causes side effects (outside of the database’s interface), and the transaction fails, then the side effects of the function will be influenced by the particular way the transaction fails, and by speculative state from partway through a transaction that was ultimately aborted rather than committed. With Fn closures, you can still smuggle speculative information out of the transaction through the use of interior mutability, but the compiler will encourage the safe default of not leaking speculative state.

Returning to retries, I think it’s reasonable for transaction to internally handle retrying upon conflict. If it didn’t, the complexity of checking the error type and conditionally retrying would be pushed out to the caller. Conflicts are a cost of doing business in databases, and if the concurrency control is good enough, the transaction should be retried and completed before long. The question of retries is coupled with the above discussion of the closure type; since the closure is Fn instead of FnMut, the API communicates that the closure run in the transaction shouldn’t have side effects.

I’m using this for now as a workaround:

fn transaction<T, E, F>(tree: &std::sync::Arc<sled::Tree>, f: F) -> Result<T, E>
where
    E: From<sled::TransactionError>,
    F: Fn(&sled::TransactionalTree) -> Result<T, E>,
{
    let res = std::cell::RefCell::new(None);

    let sled_res = tree.transaction(|tx| match f(tx) {
        Ok(value) => {
            *res.borrow_mut() = Some(Ok(value));
            Ok(())
        }
        Err(e) => {
            *res.borrow_mut() = Some(Err(e));
            Err(sled::TransactionError::Abort)
        }
    });

    let ok_value = res.into_inner().unwrap()?;
    sled_res?;
    Ok(ok_value)
}