Support stablization track futures API

[josephg]

The API for rust futures has changed a bit since the futures 0.1 crate, and the current version of the API is on track for stablization.

futures-preview exposes the new API, although internally its just re-exposing std::future. It'd be rad to support this new API in foundationdb-rs

[josephg]

I took a quick first pass to see how hard this would be. This is a sketch of the core fdb futures wrapper I've ended up with. I'm imagining the functions in tn could instead do something like this:

pub fn get(&self, key: &[u8], snapshot: bool) -> impl Future<Output=Result<Option<&[u8]>>> {
    let trx = self.inner.inner;
    let f = unsafe { fdb::fdb_transaction_get(...) };
    FdbFuture::new_mapped(f, |result| { result.get_value() })
}

... And then we could get rid of all the TrxGet / TrxGetKey / etc stuff as well. Although I also see that you're passing back a reference to the transaction itself in the get results. Is there a reason for that?

Unfortunately the futures preview TryFuture only has a map_ok function which maps X->X' not X->Result<X'>. I could make that FdbFuture struct generic over the output, but that would end up needing to generate a version of FdbFuture for each output type (of which there are a fair few). 🤷‍♀️

Anyway, the rust team is sneaking in a few changes right before they stablize futures, which is messing with the futures-preview library. We almost certainly don't want to merge any of this code until they've finished messing with / renaming AtomicWaker.

[bluejekyll]

Thanks for publishing this. I wanted to let you know that I’m currently overcoming a bout with the flu, so won’t be able to review this at the moment. Perhaps @yjh0502 would be able to review?

[josephg]

Its just a sketch-that-compiles for now - I figured it was worth figuring out a design you're happy with before porting all the older futures code over.

But look after yourself =p - I appreciate the dedication but this can wait! Go look after yourself!

[yjh0502]

@josephg Thanks for your work!

For impl Future, I agree that some of those structs could be replaced with impl Future.

If you want to write a value after a read in a transaction, you should be able to get a current transaction from read result. You can check an example code on tests [1].

About adding futures-preview support on foundationdb-rs crate, I'm a bit worry about a stability of futures-preview. I tried it a few times before, but was more unstable than I expected. I thinks it's better to add futures-preview support after things stabilize a bit more.

[1] https://github.com/bluejekyll/foundationdb-rs/blob/master/foundationdb/tests/get.rs#L18-L46

[josephg]

Yeah that makes sense. The problem with futures-preview stability is that the core team keeps making small tweaks to the API in the leadup to marking it stable. Each time the API changes, futures-preview needs retooling (which they're pretty quick about), but afterwards it only works on rust nightly versions new enough to have the API updates. This is about to happen again when they rename LocalWaker to Waker. (The std churn is because unlike futures 0.1, futures-preview uses and re-exports std::future::Future).

Once futures land in stable, futures-preview should stop breaking. (I imagine at that point it'll just become futures again). Maybe we could make a branch which uses the new futures API, and hold off merging it into master until futures reaches stable (which will probably be pretty soon - it hit final comment period in December).

The other option is to do without any of the futures combinators, but they're very useful. I'd much rather rely on AtomicWaker than write our own threading / signalling code to signal completion back to the appropriate thread.

As for that example, whoa! I've gotten so spoiled using async/await code in other languages. You have to #![feature(async_await, await_macro] but with new futures we should be able to use FDB like this:

async {
    let cluster = await!(Cluster::new(foundationdb::default_config_path()))?;
    let db = await!(cluster.create_database())?;
    await!(db.do_tn(|tn| {
        tn.set(b"hello", b"world");
    }))?;

    let value = await!(db.do_tn(async |tn| {
        await!(tn.get(b"hello"))
    }))?;
    // ...
}

[josephg]

I've gotten a hello world example working in my fork. I ended up needing to make a wrapper type for txn future results because the memory is owned by the fdb future object itself. The API currently comes out looking like this:

block_on(async {
    let cluster = await!(Cluster::new(foundationdb::default_config_path())).unwrap();
    let db = await!(cluster.create_database()).unwrap();

    await!(db.transact(async move |trx| {
        trx.set(b"hello", b"world");
        Ok(())
    })).unwrap();

    let result = await!(db.transact(|trx| trx.get(b"hello", false))).unwrap();
    assert_eq!(*result.unwrap(), b"world");
}

The *result is using the Deref trait to grab the juicy value itself inside; which lives as long as result.

This also supports running all the retry logic; which is what you basically always want as a consumer of the API.

I'm tempted to suggest we should make the transaction function Fn instead of FnMut for safety, since your function might be called multiple times if there's contention.

I've also disabled range queries, watches and almost all the old tests. But with a bit of cleanup we should be able to enable this stuff again. There should be 0 allocations / unnecessary copies in rust on the normal path, although we are unfortunately incrementing and decrementing the Arc refcount of Database and Cluster a whole lot more than we need to. But this should be pretty cheap relative to the normal network overheads of the client.