-
Notifications
You must be signed in to change notification settings - Fork 610
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use diesel-async
database pools
#9161
Conversation
619c86a
to
e8bce5e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9161 +/- ##
==========================================
- Coverage 89.17% 89.17% -0.01%
==========================================
Files 282 282
Lines 28534 28583 +49
==========================================
+ Hits 25445 25488 +43
- Misses 3089 3095 +6 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed the commits one by one with hide whitespace
turned on, and it looks good to me! Plus, the CI passed, nice works 👍
I also see that there's still a lot of work to do for the migration, especially for all the models. Let me know if you need more help. |
yeah, it's far from finished but seemed like a good first step in hopefully the right direction. I haven't found a good way yet to incrementally migrate the rest of the code. Feel free to take a look while I'm on vacation :) |
☔ The latest upstream changes (presumably 0d4928b) made this pull request unmergeable. Please resolve the merge conflicts. |
This allows us to use different implementations of `LoadConnection` (e.g. from `diesel-async`)
This allows us to use different implementations of `LoadConnection` (e.g. from `diesel-async`)
Using the `current_thread` runtime can cause deadlocks under certain conditions. This is especially visible when migrating the library to `diesel-async`.
This PR moves our codebase from
deadpool
database pools todeadpool
database pools... essentially... well... it's complicated.The codebase currently uses the database pools from
deadpool_diesel
. These database pools are async, but they still return a sync database connection that can be used with regulardiesel
queries. With this PR we are migrating to using database pools fromdiesel-async
in theirdeadpool
variant. These are returning an async connection, which can be wrapped in anAsyncConnectionWrapper
to make them work with syncdiesel
queries (in combination withspawn_blocking()
).In other words: this is just an intermediate step that allows us to use async
diesel
queries (enabled bydiesel-async
), but a lot of the code is still using sync queries with the wrapper. I've started to port over some of the simpler endpoints to use async queries, but that migration is far from complete. It should however serve as a starting point to demonstrate the end goal of removing all thespawn_blocking()
calls.Note that I had to introduce a
Conn
trait to abstract over thePgConnection
andAsyncConnectionWrapper
structs. Both implementLoadConnection<Backend = Pg>
, but since trait aliases are not stabilized yet and I didn't want to repeat that signature everywhere I decided to extract theConn
trait. After that I essentially searched for&mut PgConnection
and replaced it with&mut impl Conn
, which is equivalent, but also allows us to pass inAsyncConnectionWrapper
instead.I would recommend reviewing this commit-by-commit... 😅