Async/await and FDB 6.1 support

[dae]

I've ported this library to edition 2018, futures 0.3 and foundationdb 6.1 as a learning exercise. All the tests pass, and the bindingtester does not report any problems.

As the async changes will require a version number bump, I thought it made sense to make the breaking changes required to support FDB 6.1 at the same time, instead of going to the extra trouble of trying to support both APIs at once (#124)

If you'd like me to send a pull request let me know. In the mean time users can try it out on my branch: https://github.com/dae/foundationdb-rs/tree/futures0.3-rebased

[pH14]

Very cool -- if you have the time a PR would be great

[schallert]

Hey @dae, funny timing! I'd similarly been working on converting the library to async/await (schallert/foundationdb-rs#1) over the past few weeks before I saw you were doing the same.

It seems like our implementations are pretty similar with a few differences. One thing I noticed from the std::future docs is that

Note that on multiple calls to poll, only the Waker from the Context passed to the most recent call should be scheduled to receive a wakeup.

My understanding is that this allows a future to be moved between executors. To that end I'm using an AtomicWaker to pass to the FDB callback in order to ensure that the latest task is always woken and that we don't drop any wakes.

Would you be interesting in us comparing our approaches and trying to pull in the best of both implementations? Very cool to see someone else tackling async/await for this library :)

[dae]

Happy to collaborate! A couple of preliminary thoughts:

• Good point on AtomicWaker - I'm only using thread-local executors here, so I overlooked this. Looks like your approach solves Segfault when pending future dropped #136 as well. I wonder if waker really needs to be stored in an Arc, or whether a Box might suffice?
• I avoided async closures in my branch because they won't be in the 1.39 stable release, and I'm not sure when they will eventually land.
• It looks like the bindingtester might be failing at the moment?

[dae]

Sorry, ignore the earlier box comment - of course we also need a reference in poll().

The tests in your branch are nicer, as you've rewritten more of the code to use await instead of keeping the existing combinators in place as I did.

In terms of which branch makes a better starting point, perhaps the maintainers could chime in? If dropping pre-6.1 compatibility or accepting async closures is a dealbreaker, that may make the decision for us :-)

[schallert]

Sorry for the delay here! Yeah, my adventures down the Arc path came after some painful segfaults when just using a Box.

I hadn't realized there was a way to implement transact without async closures but happy to remove those since they seem unstable for the time being. And will touch up bindingtester too, also had trouble with that but your implementation is a helpful guide there!

[dae]

One thing that bothers me about the current db.transact() implementation is that it seems to require cloning the transaction object and other owned objects, like:

#[test]
fn test_transact_via_db() {
    common::setup_static();

    let s = String::from("test");
    let db = Database::new(foundationdb::default_config_path()).unwrap();

    block_on(db.transact(|trx| {
        let trx = trx.clone();
        let s = s.clone();
        async move {
            let trx = trx.clone();
            trx.set(s.as_bytes(), s.as_bytes());

            Ok(())
        }
    })).unwrap();
}

I wasn't able to figure out a way to modify db.transact() or the calling code to avoid the cloning, but my knowledge of lifetimes is limited, so I may be missing something.

An alternative approach that does seem to work is to move transact() into the transaction object instead, which allows for the following:

#[test]
fn test_transact_via_trx() {
    common::setup_static();

    let s = String::from("test");
    let db = Database::new(foundationdb::default_config_path()).unwrap();
    let trx = db.create_trx().unwrap();

    block_on(trx.transact(|| async {
        trx.set(s.as_bytes(), s.as_bytes());

        Ok(())
    })).unwrap();
}

Any thoughts/ideas?

[Speedy37]

I wrote my own vision of what foundationdb async/await can be: #143

I had some issue typing transact the way I wanted and I hope future rustc version will allow to remove the Pin<Box<&dyn ...>> and let me bound the lifetime of the closure more tightly.