Update to async/await

[djc]

This passes the bb8 tests on beta. postgres and redis don't compile yet, I still have figure out how to deal with Unpinness.

[khuey]

I haven't been paying a lot of attention to this so please let me know when it's ready for me to look at.

[djc]

Sure, that's what I was planning on. I've got all but one test passing in the base library, will try to redo the commits to improve review ability.

[djc]

One open question is if the closure-style run() API still makes sense in an async/await world. I think it made a lot more sense in the pre-async world where lifetime tracking on futures was very hard to impossible, but with async/await this is much better, and going back to something like r2d2-style drop-based scope might make for a more ergonomic interface. Any thoughts?

[djc]

(This is now passing all the tests in the base library. If you've got time for an early look, that might be helpful. I'll see if I can get postgres and redis working, too.)

[jebrosen]

going back to something like r2d2-style drop-based scope might make for a more ergonomic interface. Any thoughts?

I am in favor of the drop-based interface. The run() API is comparatively inconvenient for route handlers in a web application, for example: every route that uses the db must either wrap everything in a run() call, or call run() separately for every db operation (swapping connections in and out of the pool each time).

[djc]

Okay, I think this is a more or less minimal complete conversion to async/await. That is:

• bb8 passes the tests
• bb8-postgres examples compile and work
• bb8-redis compiles

Notably, for redis this still depends on an in-progress PR branch rather than a released alpha (unlike with tokio-postgres). I think there are still improvements that can be made, but I didn't want to make these commits even larger than they already are.

@khuey please review and think about the closure vs guard issue from #37 (comment).

[khuey]

The closure thing exists to make it (nearly) statically impossible to "leak" connections. I'll think about it some more but I would be a bit hesitant to give that up.

[djc]

We could potentially offer both. I do think the ergonomics of the guard type are a bit nicer. (In either case, don't need to block this PR on this question.)

[sfackler]

@khuey do you see people in practice leaking connections with something like r2d2? https://docs.rs/r2d2/0.8.6/r2d2/struct.PooledConnection.html

[khuey]

I haven't seen people leaking connections in practice with r2d2 but I also haven't been looking at how people use r2d2. In general I prefer statically checking correctness, which the closure type does (and the lack of static checking of correctness is one of my big gripes with tokio).

That said, I'm not married to it. I'm not sure when I'm going to have time to look at this PR but I'm not going to reject it just for that.

[djc]

(This PR does not change that aspect of the API -- we can do that separately.)

[khuey]

Great.

[sfackler]

You can still have that style of API if you'd like - just pass a &mut Connection into the closure rather than a connection by-value. That solves the problem of threading the connection through all the layers of error handling on the users's end.

[githubaccount624]

I'm only a user of the library and not sure of any of the technical considerations for doing so, but I would vastly prefer a drop-based interface if it's at all possible. If this were to instead require the closure style I'd probably just use tokio-resource-pool instead, since I believe they have implemented the drop-style. Also, if I recall correctly, I believe whether or not this API is made drop-based is a large consideration on whether or not it will be used for the Rocket web framework. It would integrate much more effectively if drop-based.

[khuey]

The comment about using a mut reference for the best of both worlds is pretty compelling.

…

On Tue, Nov 5, 2019, 7:21 AM githubaccount624 ***@***.***> wrote: I'm only a user of the library and not sure of any of the technical considerations for doing so, but I would vastly prefer a drop-based interface if it's at all possible. If this were to instead require the closure style I'd probably just use tokio-resource-pool instead, since I believe they have implemented the drop-style. Also, if I recall correctly, I believe whether or not this API is made drop-based is a large consideration on whether or not it will be used for the Rocket web framework. It would integrate much more effectively if drop-based. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#37>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACPSBBK7A6W6Y3PNXKZBPDQSGFRFANCNFSM4JADDOSQ> .

[bbigras]

I'm not sure if it's a bug, but if is_valid() returns an error, bb8 doesn't try to set up a new connection and I get a TimedOut error after 30 secs.

rustc 1.40.0-nightly (b520af6fd 2019-11-03)

[dependencies]
async-trait = "0.1.17"
bb8 = { git = "https://github.com/djc/bb8", branch = "async-await" }
env_logger = "0.7.1"
log = "0.4.8"
tokio = { package = "tokio", version = "0.2.0-alpha.6" }

use async_trait::async_trait;
use bb8::ManageConnection;
use env_logger::Env;
use log::{debug, error};

use std::io::{Error, ErrorKind};

struct Connection {}
struct Manager {}

#[async_trait]
impl ManageConnection for Manager {
    type Connection = Connection;
    type Error = Error;

    async fn connect(&self) -> Result<Self::Connection, Self::Error> {
        debug!("connect()");
        Ok(Connection {})
    }

    async fn is_valid(
        &self,
        conn: Self::Connection,
    ) -> Result<Self::Connection, (Self::Error, Self::Connection)> {
        debug!("is_valid()");

        Err((Error::new(ErrorKind::Other, "oh no!"), conn))
    }

    fn has_broken(&self, _conn: &mut Self::Connection) -> bool {
        debug!("has_broken()");
        false
    }
}

#[tokio::main]
async fn main() -> Result<(), Error> {
    let env = Env::default().filter_or("RUST_LOG", "debug");
    env_logger::init_from_env(env);

    let pool = bb8::Builder::new().max_size(1).build(Manager {}).await?;

    for i in 1..=2 {
        debug!("try: {}", i);

        if let Err(e) = pool
            .run(|c: Connection| {
                async {
                    debug!("use connection");
                    Ok::<(u8, Connection), (Error, Connection)>((1, c))
                }
            })
            .await
        {
            error!("err pool.run: {}", e);
        }
    }

    Ok(())
}

log

[2019-11-08T20:38:22Z DEBUG test_bb8] try: 1
[2019-11-08T20:38:22Z DEBUG test_bb8] connect()
[2019-11-08T20:38:22Z DEBUG test_bb8] use connection
[2019-11-08T20:38:22Z DEBUG test_bb8] has_broken()
[2019-11-08T20:38:22Z DEBUG test_bb8] try: 2
[2019-11-08T20:38:22Z DEBUG test_bb8] is_valid()
[2019-11-08T20:38:52Z ERROR test_bb8] err pool.run: Timed out in bb8

[djc]

Thanks for trying it! Could you please turn this into a commit that adds a test case? That makes it easier to work on and make sure we don't regress that case going forward.

[bbigras]

Thanks for trying it! Could you please turn this into a commit that adds a test case? That makes it easier to work on and make sure we don't regress that case going forward.

Done #1

[djc]

@bbigras are you sure the test is right? The condition in is_valid() seems off, in that it seems like the !conn.once case should return Ok instead of Err.

[bbigras]

@djc in the test, I wanted is_valid() to fail but only once. To make sure bb8 retries once.

The once attributes should have been named failed_once. It starts at false and becomes true after failing once.

[djc]

Ah, ok.

[bbigras]

I just realized in the shower that it wont work since the connection whould be new after a fail. So it will always fail.

I should use an atomic int or something like that.

[bbigras]

Done. #2

[elpiel]

One thing to note: futures 0.3 have been release after Rust 1.39 got released

[djc]

Right, but there is no tokio (alpha) release that can work with futures 0.3 yet.

[elpiel]

@djc are you interested in mentoring for the drop-based interface changes?

[djc]

@elpiel sure, if you want to implement it I'm happy to answer your questions and/or give you guidance.

[elpiel]

Missing an .await

[elpiel]

Missing an .await

[djc]

@khuey is there anything I can do to make this easier to review? I'm also okay with becoming co-maintainer if you'd like. As it is, I find myself not very motivated to make further changes until I get some feedback on the progress here. :)

[khuey]

Reviewing this is on my todo list for this weekend.

[khuey]

I have a couple small concerns that I'll file followup issues on but nothing that I think should block landing this.

Thank you very much for contributing this and for your patience.

[djc]

@khuey thanks for merging, no problem with the waiting -- I know how it goes.

Feel free to cc me on any follow-up issues you file.