Notes from moving from diesel to diesel_async in a small codebase

[Heliozoa]

Hi,
Thanks for your work on diesel.
I wrote some notes on stuff I came across when moving from diesel 2.0.0 to diesel-async 0.1.0 in a small (~6000 LOC) private codebase.

• My diesel imports were previously just use diesel::prelude::* with the exception of use diesel::result::Error as DieselError; in an error module. I think it'd be nice to have an equivalent in diesel-async that is similar to diesel's prelude but replaces/adds the relevant traits and types from diesel-async. Maybe even include FutureExt for boxed.
• Functions that previously took borrowed arguments and ran transactions turned out to be tricky. I understand that I can't give the async transaction any temporary borrows because there's no guarantee I will actually run the future to completion within the same function. Not sure what the best workaround here is beyond just using owned values. That was a little awkward as well, as previously I just took &mut PgConnection, which worked with both pooled and unpooled connections (used mostly in tests) thanks to some Deref impl, but the naive owned version now takes Object<AsyncPgConnection> which always requires the connection to be pooled. I could make the functions generic but I'm not sure that'd be worth the trouble. (edit: This turned out to not be an issue, I just mixed something up while debugging other problems) I guess the long term solution is to wait for async scopes...
• I had a brief moment of confusion over the deadpool Pool and Object type aliases including the AsyncDieselConnectionManager, mainly because the equivalent diesel type aliases don't do it. Not a big deal either way, just something I noticed.
• I didn't get a clear idea from the benchmarks how diesel-async compares performance-wise to my current use of diesel, which is always with tokio::task::spawn_blocking. Any idea? If not or just a vague one, that's fine too.

[mohe2015]

Can totally agree with the transactions lifetime issue point. Would it maybe be possible to provide an api like:

let tx = connection.begin().await?;
....execute(&mut tx).await?;
drop(tx);

[weiznich]

Thanks for reporting back those experience. Let me try to address a few of those points:

• My diesel imports were previously just use diesel::prelude::* with the exception of use diesel::result::Error as DieselError; in an error module. I think it'd be nice to have an equivalent in diesel-async that is similar to diesel's prelude but replaces/adds the relevant traits and types from diesel-async. Maybe even include FutureExt for boxed.

I would prefer to "require" something like use diesel::prelude::*; for the future as well, just to make clear that most of the functionality actually comes from diesel itself. For my own projects I tend to just do the following:

use diesel::prelude::*;
use diesel_async::{RunQueryDsl, AsyncPgConnection};

(Note that named imports always "overwrite" wildcard imports, so rustc will use RunQueryDsl from diesel_async.)
The same is mostly true for reexporting FuturesExt.

• Functions that previously took borrowed arguments and ran transactions turned out to be tricky. I understand that I can't give the async transaction any temporary borrows because there's no guarantee I will actually run the future to completion within the same function. Not sure what the best workaround here is beyond just using owned values. That was a little awkward as well, as previously I just took &mut PgConnection, which worked with both pooled and unpooled connections (used mostly in tests) thanks to some Deref impl, but the naive owned version now takes Object which always requires the connection to be pooled. I could make the functions generic but I'm not sure that'd be worth the trouble. I guess the long term solution is to wait for async scopes...

I'm not sure I can really follow here. Can you provide an example for this. I would assume that the transaction callbacks work similar than in "normal" diesel, at least in terms of using the connection.

• I didn't get a clear idea from the benchmarks how diesel-async compares performance-wise to my current use of diesel, which is always with tokio::task::spawn_blocking. Any idea? If not or just a vague one, that's fine too.

As this is the first release of diesel-async at all, there are no final numbers on that yet. From the benchmarks over at the main diesel repository it looks like diesel-async shows a similar performance than ordinary diesel. Note that these benchmarks only measure the raw database connection part. How that translates to more complex setups (like webservers) is not clear for me, but I wouldn't expect any problems as long as you don't expect huge amounts of traffic.

Can totally agree with the transactions lifetime issue point. Would it maybe be possible to provide an api like:

let tx = connection.begin().await?;
....execute(&mut tx).await?;
drop(tx);

I'm opposed to such an API as it makes it much harder to somehow handle the case where users forget to explicitly commit/rollback a transaction.

[Heliozoa]

I would prefer to "require" something like use diesel::prelude::*; for the future as well, just to make clear that most of the functionality actually comes from diesel itself.

I see. That's understandable, though I still would prefer a prelude myself because diesel is such a "fundamental" crate in projects where I use it and a lot of its functionality comes from traits, so it was nice to just be able to do the prelude import and not worry about the details like the fact that transaction comes from AsyncConnection.

Can you provide an example for this

(edit: Sorry for the noise, I guess using &mut AsyncPgConnection is fine actually, I must have had issues with it when trying to figure out a workaround for borrowed arguments issues.)

Sure, here's a simplified example from my project (the real code is pretty much the same but just inserts into a couple other tables as well)

use diesel::prelude::*;

pub fn transaction(
    conn: &mut PgConnection,
    tag_id: i32,
    language: Option<&str>,
    description: Option<&str>,
) -> QueryResult<i32> {
    use crate::schema::tag_details as td;

    conn.transaction(|conn| {
        let details_id = diesel::insert_into(td::table)
            .values((
                td::tag_id.eq(tag_id),
                td::description.eq(description),
                td::language.eq(language),
            ))
            .returning(td::id)
            .get_result(conn)?;

        QueryResult::<_>::Ok(details_id)
    })
}

Following the example at https://docs.rs/diesel-async/0.1.0/diesel_async/trait.AsyncConnection.html#method.transaction my translation of that into diesel-async looks like this

use diesel::prelude::*;
use diesel_async::{AsyncConnection, AsyncPgConnection, RunQueryDsl};
use futures::FutureExt;

pub async fn transaction(
    conn: &mut AsyncPgConnection,
    tag_id: i32,
    language: Option<&str>,
    description: Option<&str>,
) -> QueryResult<i32> {
    use crate::schema::tag_details as td;

    conn.transaction(|conn| {
        async move {
            let details_id = diesel::insert_into(td::table)
                .values((
                    td::tag_id.eq(tag_id),
                    td::description.eq(description),
                    td::language.eq(language),
                ))
                .returning(td::id)
                .get_result(conn)
                .await?;

            QueryResult::<_>::Ok(details_id)
        }
        .boxed()
    })
    .await
}

which causes the following error

error: lifetime may not live long enough
  --> src/domain/tags.rs:16:9
   |
10 |       language: Option<&str>,
   |                        - let's call the lifetime of this reference `'1`
...
16 | /         async move {
17 | |             let details_id = diesel::insert_into(td::table)
18 | |                 .values((
19 | |                     td::tag_id.eq(tag_id),
...  |
28 | |         }
29 | |         .boxed()
   | |________________^ returning this value requires that `'1` must outlive `'static`
  --> src/domain/tags.rs:16:9
   |
11 |       description: Option<&str>,
   |                           - let's call the lifetime of this reference `'2`
...
16 | /         async move {
17 | |             let details_id = diesel::insert_into(td::table)
18 | |                 .values((
19 | |                     td::tag_id.eq(tag_id),
...  |
28 | |         }
29 | |         .boxed()
   | |________________^ returning this value requires that `'2` must outlive `'static`

How that translates to more complex setups (like webservers) is not clear for me, but I wouldn't expect any problems as long as you don't expect huge amounts of traffic.

Yeah, it's not really something I'm worried about, just curious if moving to diesel-async in an async codebase is mainly for improved ergonomics or if I should expect a performance benefit (even if small) as well due to not having to create a task for each query.

[weiznich]

So the "easy" fix for the transaction problem is to not use reference values in your function signature. The following code works as expected:

pub async fn transaction(
    conn: &mut AsyncPgConnection,
    tag_id: i32,
    language: Option<String>,
    description: Option<String>,
) -> QueryResult<i32> {
    use crate::schema::tag_details as td;

    conn.transaction(|conn| {
        async move {
            let details_id = diesel::insert_into(td::table)
                .values((
                    td::tag_id.eq(tag_id),
                    td::description.eq(description),
                    td::language.eq(language),
                ))
                .returning(td::id)
                .get_result(conn)
                .await?;

            QueryResult::<_>::Ok(details_id)
        }
        .boxed()
    })
    .await
}

Issues like that one are the reason why I personally do not consider the async ecosystem ready for such code. The underlying issue is again that we cannot really express the corresponding lifetimes of the transaction callback.

[Heliozoa]

Got it, I agree with that.

While trying to debug the issue, I came across this crate: https://docs.rs/async-scoped/latest/async_scoped/. I wonder if diesel-async could build a safe transaction_scoped with it, or with a similar strategy? At least for me, this is a pretty significant pain point. That said, I'm also totally satisfied with just using regular diesel and waiting for wg-async to do its thing no matter how long it takes.

[weiznich]

Something like async_scoped does likely not help. The underlying issue is that the transaction callback needs to take an argument (the connection), while most async API's that somehow "schedule" an future are take the future itself as argument. That makes it much easier to reason about lifetimes. Unfortunately does this not work for the transaction case, as you need to perfom database calls before and after the transaction "callback". See this playground for a simplified example. It's really nothing that's simple to solve, I try that since a few years already. Also it gets even harder if you start to think about what needs to happen when users cancel the future returned from transaction function.

[djflex68]

I'm opposed to such an API as it makes it much harder to somehow handle the case where users forget to explicitly commit/rollback a transaction.

In fact, i'm facing an issue since a couple of days, which seems only to have a solution using a low-level API to access the transaction_manager directly.
In my case I'm trying to implement the UnitOfWork pattern aka TransactionScope. The idea is to make a whole service business method transactional, so that everything that runs inside this business method will share the same transaction under the hood (imagine for example a business method that should create an order in db and debit a customer card, two business operations that need to be transactional)
Technical considerations about transactions and connections are commonly handled inside Repository classes that directly communicate with an ORM like diesel.
As rules of thumb, we shoud then never :

• have any reference to a technical framework (orm, dB connection in , db transaction) in a service layer
• code more than necessary in the repository layer because of purely technical reasons (for example by bringing a bunch of business code in the repository layer where it has nothing to do, just because we need to make the code transactional).

So back to the UnitOfWork. I've implemented it, pretty much the same way the run method is defined today. You call UnitOfWork.create(...) and you give the create method an async closure that accept a RepositoryFactory as parameter. This RepositoryFactory has a get_repository method which returns an instance of a specific repository by transparently injecting the connection in the constructor of the created Repository instance.
Under the hood the implementation of the UnitOfWork::create method just gets a PooledConnection out of a BB8 pool, then calls build_transaction and run. Inside the closure of run, I then instantiate a RepositoryFactory with the mutable reference to conn given by run method, and pass this RepositoryFactory to the original closure (the one given in UnitOfWork.create method).
The excepted result would be that service layer methods just do something like that :

aync fn my_business_method_that_acts_transactionally() {
    UnitOfWork.create(|repository_factory| {
        async move {
              repository_factory.get_repository::<MyRepository>().save()
              repository_factory.get_repository::<MyOtherRepository>().update()
        }
    }

Both save operations are handled transactionally without any reference to an ORM framework and the connection is managed by RepositoryFactory and passed around to created Repository instances when calling get_repository().
From what I've understood, the whole problem comes from the mutable reference to conn that is retrieved inside run method and that cannot escape the closure scope (I've tried so much things with Arc, dereferencing and so on..). The only thing that would work is not having a closure at all and being able to start and commit/rollback the transaction myself, pretty much exactly what you are doing in the implementation of the run method.
I hope this real-life use case can help improving the library, I haven't found any workaround so far.
I will post the real code here tomorrow when I'm back to the office 😄

[djflex68]

Thanks for this answer, this helps indeed 👍 Using directly AnsiTransactionManager for instance might solve my issue. 😄

By the way, sorry for that question that probably feels stupid, but what happens if we are in the very unluckily case, where the system crashes/shutdowns exactly between begin_transaction and commit_transaction ?

• Will a pending transaction remains, blocking the row/table ?
• Does diesel "know" when starting a new transaction, that another one has not be committed successfully and try to unlock it ?
• How can a developer "unlock" such a situation of an uncommitted transaction, since your user-friendly "run" callback performs "begin, commit and rollback" ?
• Should the developer test the state of the transaction and try to rollback manually ?

Sorry if this is not the appropriate topic or appears to be some silly questions, and thanks again for you help 😕

[weiznich]

By the way, sorry for that question that probably feels stupid, but what happens if we are in the very unluckily case, where the system crashes/shutdowns exactly between begin_transaction and commit_transaction ?

That's not a silly question. It actually a really good question to think about.

Now it depends a bit on what you are exactly understanding here under "crashes". If that means bringing down the whole connection/application that's fine, because database systems then just drop the transaction at some point.
If that means that you just cancel the underlying future (timeout, …) and reuse the connection later on it becomes more complicated. If you use begin_transaction manually it's completely up to you to handle this case. If you use AsyncConnection::transaction it will just leak the transaction in that case, because stable rust does not offer a good cay to either specify that a given future must run to completion nor to perform an async action on Drop. (That's an issue for all async database imlementation). It essentially means that you can end up with a pending transaction even if you use AsyncConnection::transaction. In cases where you use one of the built-in pooling solution diesel-async automatically checks for open transactions if the connection is returned to the pool. In this cases the transaction is considered broken and will be closed (which rolls back the transaction as well).

[djflex68]

Thanks for this precision 👍

In cases where you use one of the built-in pooling solution diesel-async automatically checks for open transactions if the connection is returned to the pool. In this cases the transaction is considered broken and will be closed (which rolls back the transaction as well).

This means that if I use a pooled connection (like bb8 pool), I don't really have to worry about a pending transaction remaining, because in all cases the transaction is rollbacked, either :

• by the client, if the error is catched correctly (general case)
• by the server, if the program crashes and therefore there is no established connection anymore
• by diesel, if the program does not crash, but the future is cancelled, and the pool/diesel manages that case

So using a pooled connection makes the manual usage of begin_transaction/commit_transaction safe in all cases, if I understand it correctly ?

And admitting I would succeed in using the typical use-case scenario of building a transaction with conn.build_transaction().run::<_, diesel::result::Error, _>(|conn| { :

• We should really only use a PooledConnection for conn here, and never an AsyncPgConnection directly, because in that case, even the run closure will not rollback a pending transaction where the future has been cancelled, since the run method implementation is exactly the same :

 AnsiTransactionManager::begin_transaction_sql(&mut *self.connection, &sql).await?;
        match f(&mut *self.connection).await {
            Ok(value) => {
                AnsiTransactionManager::commit_transaction(&mut *self.connection).await?;
                Ok(value)
            }
            Err(e) => {
                AnsiTransactionManager::rollback_transaction(&mut *self.connection).await?;
                Err(e)
            }
        }

Is that true ?

[weiznich]

It's almost correct to state it that way.

So using a pooled connection makes the manual usage of begin_transaction/commit_transaction safe in all cases, if I understand it correctly ?

I wouldn't say all cases here, but restrict that to it's safe to assume that any connection that you got freshly from the pool does not contain an open transaction (at least as long as you did not provide a pool customization that explicitly opens a connection there).

We should really only use a PooledConnection for conn here, and never an AsyncPgConnection directly, because in that case, even the run closure will not rollback a pending transaction where the future has been cancelled, since the run method implementation is exactly the same :

That's not true in that totally. It's fine to use a &mut AsyncPgConnection that was obtained via DerefMut from the pooled connection. It's problematic to use a AsyncPgConnection directly constructed with AsyncConnection::establish without pool.

[djflex68]

It's all clear now, many thanks for you support 🙏

[jeff-hiner]

Wanted to add that the initial post has pretty much mirrored my experience trying to port existing diesel code. I think there's at least some headaches coming from the compiler's lack of async lifetime support, some from the rough ergonomics around forced use of boxing and async_trait, and a little bit of confusion over which features need to be enabled when combining diesel_async with diesel with a pool crate.

1. Not having a diesel_async::prelude caused a lot of problems as I chased down conflicting implementations of PgConnection and RunQueryDsl. "Just import diesel::prelude and override what you need" is a good idea in theory for brand new code, but makes for some really confusing conflicts in existing code that imports the "wrong" synchronous types. I also had a custom paginator that would have required turbofish everywhere because of conflicting trait impls. I wound up creating my own internal prelude and including it everywhere, which I think many other users will have to do. I'll echo the prior comment that a bundled prelude would help enormously with porting, even with a rustdoc caveat that nearly all of it is just reimports from diesel::prelude.
2. Lifetime issues make it impossible to use Pool type objects with test_transaction, which unfortunately isn't documented in the rustdoc. This might seem like a nonissue, just use a single connection instead of a pool. But in our case there's a lot of existing code that takes non-generic pool connections, and we still need to test it all. The workaround for this is using begin_test_transaction (which has no rustdoc?) and then just dropping the connection when done.

[weiznich]

Thanks for providing this feedback. That's really helpful.

I think there's at least some headaches coming from the compiler's lack of async lifetime support, some from the rough ergonomics around forced use of boxing and async_trait,

That's just the state of the ecosystem. Hopefully that gets fixed at language level at some point...

and a little bit of confusion over which features need to be enabled when combining diesel_async with diesel with a pool crate.

Essentially you just need to depend on a compatible version of diesel. There is no need to enable any features on the "main" diesel crate as long as you enable the relevant features on diesel_async. Would it be help if the API documentation would contain that information?

Not having a diesel_async::prelude caused a lot of problems as I chased down conflicting implementations of PgConnection and RunQueryDsl. "Just import diesel::prelude and override what you need" is a good idea in theory for brand new code, but makes for some really confusing conflicts in existing code that imports the "wrong" synchronous types. I also had a custom paginator that would have required turbofish everywhere because of conflicting trait impls. I wound up creating my own internal prelude and including it everywhere, which I think many other users will have to do. I'll echo the prior comment that a bundled prelude would help enormously with porting, even with a rustdoc caveat that nearly all of it is just reimports from diesel::prelude.

I think you might be right here that having a prelude in diesel_async that reexports from diesel is the long term way to go. I see one big fundamental problem with that approach, that's why I've not implemented it yet: It will cause issues will all the derive macros, as they assume that there is a crate called diesel as direct dependency of your crate. If we now would have a diesel_async::prelude module that reexport the prelude from diesel with additions from diesel_async that would cause two issues:

• You likely wouldn't directly depend on diesel anymore as diesel_async now as "anything" you as a user need.
• The derives would be broken as there is no direct dependency named diesel anymore

It works to have such a prelude in your crate, as you still depend on diesel itself in that case.

Lifetime issues make it impossible to use Pool type objects with test_transaction, which unfortunately isn't documented in the rustdoc. This might seem like a nonissue, just use a single connection instead of a pool. But in our case there's a lot of existing code that takes non-generic pool connections, and we still need to test it all. The workaround for this is using begin_test_transaction (which has no rustdoc?) and then just dropping the connection when done.

Can you provide a few more details on this? Both AsyncConnection::test_transaction and AsyncConnection::begin_test_transaction are documented in the API documentation, so it's unclear what that rustdoc part is referring to. For the lifetime issues it would be great to have a somewhat minimal example that demonstrates the problem.

[jeff-hiner]

Essentially you just need to depend on a compatible version of diesel. There is no need to enable any features on the "main" diesel crate as long as you enable the relevant features on diesel_async. Would it be help if the API documentation would contain that information?

That's accurate for connectors, but we still needed to enable features within diesel for types and serialization within our schema: "chrono", "numeric", "serde_json", "uuid". For the pools, specifically more information about the pool crates would be useful. diesel_async has features for postgres and deadpool which we wound up eventually selecting over bb8, but it wasn't clear what features if any needed to be included in our deadpool import. There are entire ecosystems of "glue" crates like deadpool-postgres and tokio-postgres that also cause confusion. Maybe a short explanation of which ones are never needed, vs which are sometimes needed (e.g. if you're implementing a TLS connector).

would cause two issues:

• You likely wouldn't directly depend on diesel anymore as diesel_async now as "anything" you as a user need.
• The derives would be broken as there is no direct dependency named diesel anymore

This is an issue that serde_derive used to have, and yeah it can be a bit annoying.

To the first, there are still often bits required from diesel that aren't in the prelude, so I expect a lot of folks will still naturally pull in diesel for struct macros like diesel::table! and operators like diesel::delete. As to the second, the current documentation is pretty clear that "you need to include diesel in your Cargo.toml and I'd expect that wouldn't need to change.

Can you provide a few more details on this? Both AsyncConnection::test_transaction and AsyncConnection::begin_test_transaction are documented in the API documentation, so it's unclear what that rustdoc part is referring to. For the lifetime issues it would be great to have a somewhat minimal example that demonstrates the problem.

Weird, I must have been looking at some stale rustdoc because you're right, that is pretty clearly documented there.

The easiest way to reproduce the lifetime scoping issues is to use the code in the test_transaction rustdoc example, but replace the hidden boilerplate connection logic with a bb8::Pool and associated .get().

let config = AsyncDieselConnectionManager::<diesel_async::AsyncPgConnection>::new(
    "postgres://localhost:5432",
);
let pool = Pool::builder().build(config).await.unwrap();
let mut conn = pool.get().await.unwrap();

I copied the rustdoc code into a fresh project (adjusting the paths to compile otherwise), and I get these. Strangely, using transaction instead of test_transaction works just fine.

error[E0597]: `conn` does not live long enough
  --> src/main.rs:31:9
   |
29 |         let mut conn = pool.get().await.unwrap();
   |             -------- binding `conn` declared here
30 |
31 |         conn.test_transaction::<_, Error, _>(|conn| {
   |         ^^^^ borrowed value does not live long enough
...
50 |     }
   |     -
   |     |
   |     `conn` dropped here while still borrowed
   |     borrow might be used here, when `conn` is dropped and runs the `Drop` code for type `bb8::api::PooledConnection`

error[E0499]: cannot borrow `conn` as mutable more than once at a time
  --> src/main.rs:48:59
   |
31 | ...   conn.test_transaction::<_, Error, _>(|conn| {
   |       ---- first mutable borrow occurs here
...
48 | ...   let all_names = users.select(name).load::<String>(&mut conn).await...
   |                                                         ^^^^^^^^^
   |                                                         |
   |                                                         second mutable borrow occurs here
   |                                                         first borrow later used here

https://github.com/jeff-hiner/diesel_async_tests/blob/main/src/main.rs#L25-L29

[weiznich]

Thanks for coming back to provide these answers.

 That's accurate for connectors, but we still needed to enable features within diesel for types and serialization within our schema: "chrono", "numeric", "serde_json", "uuid". 

Right, I totally forgot that these features might be relevant while writing my answer. Yes it would be good to be explicit about that in the documentation.

For the pools, specifically more information about the pool crates would be useful. diesel_async has features for postgres and deadpool which we wound up eventually selecting over bb8, but it wasn't clear what features if any needed to be included in our deadpool import. There are entire ecosystems of "glue" crates like deadpool-postgres and tokio-postgres that also cause confusion. Maybe a short explanation of which ones are never needed, vs which are sometimes needed (e.g. if you're implementing a TLS connector).

As far as I'm aware you don't need any of these "glue" crates. You might need the "root" crate, so deadpool/bb8/… depending on your needs. Maybe that should be pointed out in the relevant documentation? I felt that this would would be covered by the examples in the documentation: https://docs.rs/diesel-async/latest/diesel_async/pooled_connection/deadpool/index.html (they explicitly shows the required imports).

That written: I'm always happy to merge PR's that improve the documentation.

For the other issue: I will try to have a look at that later this week.