Sled: Tree::transaction API prevents error forwarding

[theduke]

The function passed to Tree::transaction must currently return a Result<_, TransactionError>.

This prevents users from forwarding business logic errors that occur during a transaction.

I can see two solutions:

• make the error generic, so it is under the control of the user. diesel does it this way

• Add a TransactionError::Custom(Box<dyn std::error::Error) variant that can be used.

[theduke]

Unfortunately, both solutions are a breaking change. (A obviously is, but B is also breaking because TransactionError does not use the non-exhaustive hidden variant trick.

[Kerollmops]

I have found a way to forward business logic errors:

let mut error = None;

// start of the transaction
    if an error occured do {
        error = Some(e);
    }
// end of the transaction

if let Some(e) = error {
    return Err(e);
}

Even better you can simulate a try block while it is not stabilized:

let result = (|| {
    fallible_call()?;
})();

EDIT: sadly the transaction function is not FnMut therefore can not modify external context.

[theduke]

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)
}

[theduke]

As an additional note: transaction should take FnOnce, not Fn.

[divergentdave]

I think Fn is correct, the closure may need to be run more than once, to retry in the event of conflicts. The function call is in a loop, and it can hit a continue after the call if validation fails.

[Kerollmops]

So FnMut would have been better, than Fn no? This way it would be possible to modify captured variables.

[theduke]

@divergentdave the function might have side effects. A default transaction definitely should not do automatic retries.

There could be a additional transaction_retry(..., max_retries: usize) -> ... method.

[divergentdave]

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.

[spacejam]

It's Fn because once we implement optimistic concurrency control, it might be retried transparently.

Your Ok value may be any type. It's generally a poor habit to make errors that squeeze all concerns together, because then you have to think about all of them any time they are handled. Tree's cas method used to not be a nested result, and there was a sled Error variant for cas failures. This felt nice at first because of the avoidance of nesting, but it led to a lot of bugs over time because the types were not specific enough to be handled in clean ways.

I am not sure if it's a good idea to add this variant to TransactionError because it feels like it's running into the same territory where handling of IO errors is clumped into the same place as user-level concerns.

[spacejam]

Is this issue more that you just don't like the nesting? There should be no restriction that prevents users from propagating their own Results in the success type of TransactionResult.

[theduke]

I agree that a variant on TransactionError is not a good design, I'd rather see a method signature like in diesel (as linked above).

The issue is that a lot of business logic errors can occur during a transaction, and you want to forward and handle those errors appropriately. Using TransactionError::Abort doesn't tell you anything about what went wrong.

[0xpr03]

TransactionError::Abort(Option<T>) ?
This could default to None, backwards compatible.
But allows a custom error type.

[spacejam]

I've spent some hours throwing a bunch of different options at the wall, and after trying out different approaches that try to convert to a error automatically as well as approaches that allow a specific type to be passed through an abort, I've landed at the latter for now. What do you folks think about it? https://github.com/spacejam/sled/pull/861

[spacejam]

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.

[spacejam]

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 :)