Errors in db.transact() closure are not retried

[eastern-grey]

Hi guys,

I'm seeing an issue where some transactions run with db.transact() are timing out with "Error { kind: \n\nFoundationDB error(1009): Request for future version, should_retry: true }" after 1 second, when a storage server gets a little lagged. I'm puzzled as to why the transaction is not retried, as I'm not setting any transaction options, and I gather there are no timeouts by default. I don't have any experience using foundationdb in other language bindings, so I'm not sure if this is an issue with the rust bindings or in foundationdb itself. If it's the latter, please let me know and I'll take the issue up on the foundationdb forums.

[eastern-grey]

It looks like db.transact is only checking the error returned by commit, and not any errors during get()s that the closure may return. This seems to be different behaviour to the other language bindings?

The following implementation seems to work.I'm very new to rust, so it probably could be done better.

pub async fn transact<F, Fut, Output>(db: &Database, func: F) -> Result<Output>
    where
        F: Fn(Transaction) -> Fut,
        Fut: Future<Output = Result<Output>>,
{
    let trx = db.create_trx().unwrap();

    loop {
        trx.set_option(TransactionOption::Timeout(5000))?;

        let res = func(trx.clone()).await;

        // did the closure return an error?
        if let Err(e) = res {
            let res = e.downcast::<foundationdb::error::Error>();
            if let Ok(e) = res {
                trx.on_error(e).compat().await?;
                continue;
            }

            // non-fdb error, abort
            trx.cancel();
            return Err(res.unwrap_err());
        } else {
            // commit
            match trx.clone().commit().compat().await {
                // and return the value from the closure on success
                Ok(_) => return res,
                Err(e) => {
                    trx.on_error(e).compat().await?;
                }
            }
        }
    }
}

[pH14]

Ooh good find, it looks like the commit's result is remapped to the result of the function call.

https://github.com/bluejekyll/foundationdb-rs/blob/92c30c93eec3714c3015852aa9dc3cf9d8f993df/foundationdb/src/database.rs#L85-L87

I think it's the commit's error that might be swallowed as a result. That probably means a goodly number of errors are not being retried, since so many checks occur at commit time. @eastern-grey interested in tossing up a PR? Since async/await aren't stable yet, we might want to avoid them for now, but the bones of your changes shouldn't be hard to adapt.

[bluejekyll]

@pH14 since you've taken such an active role in this (and me less so recently), I've added you as a collaborator. Let me know if you want access to push crates as well.

[eastern-grey]

I'm afraid I struggle with the 0.1 future combinators :-(

If my understanding is correct, commit errors are being retried (though without the backoff that on_error() would give), and the issue is that when f() returns an Err, .and_then() never gets called, and the Err is returned to the loop.

Hopefully async is not far off, as it makes things a lot easier. Looks like they're aiming for a beta next month: rust-lang/rust#62149

[pH14]

Aha, yes that makes sense, I was misunderstanding how a few of the combinators worked (still a Rust newbie!). An async rewrite of many of these components would be quite nice :)

[dae]

I've implemented a tweaked version of this in #135. The on_error() calls are actually unnecessary, as the backoff is applied when a TrxErrFuture is returned.