djc / bb8

Full-featured async (tokio-based) postgres connection pool (like r2d2)
MIT License
753 stars 110 forks source link

Remove async-trait in favor of impl trait and native async in impls #196

Open svix-jplatte opened 6 months ago

svix-jplatte commented 6 months ago

Hi! I would like to propose to redefine the ManageConnection and CustomizeConnection traits from

#[async_trait]
pub trait ManageConnection: Sized + Send + Sync + 'static {
    // other items...

    async fn connect(&self) -> Result<Self::Connection, Self::Error>;
    async fn is_valid(&self, conn: &mut Self::Connection) -> Result<(), Self::Error>;
}

#[async_trait]
pub trait CustomizeConnection<C: Send + 'static, E: 'static>:
    fmt::Debug + Send + Sync + 'static
{
    async fn on_acquire(&self, _connection: &mut C) -> Result<(), E> {
        Ok(())
    }
}

to

pub trait ManageConnection: Sized + Send + Sync + 'static {
    // other items...

    fn connect(&self) -> impl Future<Output = Result<Self::Connection, Self::Error>> + Send;
    fn is_valid(&self, conn: &mut Self::Connection) -> impl Future<Output = Result<(), Self::Error>> + Send;
}

pub trait CustomizeConnection<C: Send + 'static, E: 'static>:
    fmt::Debug + Send + Sync + 'static
{
    fn on_acquire(&self, _connection: &mut C) -> impl Future<Output = Result<(), E>> + Send {
        async move { Ok(()) }
    }
}

for bb8 0.9.0.

This will allow bb8 to be used without pulling in any proc-macros AFAICT, people will still be able to write async fn in impls (just without #[async_trait]), and it will improve the rendered documentation (see https://github.com/svix/omniqueue-rs/pull/34 for the diff on another crate).

I can send a PR if you agree that this is a good idea. In that case, do you want to keep main for the 0.8.x release line and create a new branch for 0.9.x, or have the PR against main?

djc commented 6 months ago

Would this need an MSRV bump, and if so, to what version? This is just RPITIT, right? Why not use AFIT here?

I do think it would be nice to adopt a more direct API eventually and am open to a PR, but unsure about the timeline here. It feels a little silly to adopt RPITIT now only to move over to AFIT later?

svix-jplatte commented 6 months ago

Would this need an MSRV bump, and if so, to what version?

Yes, this would bump the MSRV to 1.75.

This is just RPITIT, right? Why not use AFIT here?

This is both, because they're compatible. RPITIT for the trait declarations to require Send futures, otherwise code that's generic over these traits has to assume !Send futures until RTN (return-type notation) comes around, which would be a PITA; impls can be written using AFIT.

I do think it would be nice to adopt a more direct API eventually and am open to a PR, but unsure about the timeline here. It feels a little silly to adopt RPITIT now only to move over to AFIT later?

Yeah, it would be a breaking change to move the declarations to AFIT later, but a very minor one because implementations wouldn't change at all, just code that's generic over these traits and assumes the returned futures are Send would have to use RTN or some equivalent once the Send requirement is removed from the trait decl (with or without switching from RPITIT to AFIT).

djc commented 6 months ago

Okay, it will be a while before I want to accept a bump to 1.75, probably when we hit 1.81 or 1.83 or some such?

svix-jplatte commented 6 months ago

Okay, so... In half a year or a bit more. That's seems like a lot to me but I'm not here to start a debate over MSRV policies :)

Thanks for the fast responses!

djc commented 6 months ago

Yes, sorry -- I know I am on the conservative side here -- and given that this is definitely a lower-priority project for me I really don't want to maintain two branches in parallel.

svix-jplatte commented 6 months ago

No worries, it's not like async-trait is a seriously problematic dependency to get rid of.