search

overdrivenpotato / bb8-diesel

bb8 connection manager for Diesel
MIT License
21 stars 12 forks source link

bb8-diesel cannot be used within a tokio `LocalSet` (or work within `actix_web`) #1

Open agersant opened 3 years ago

agersant commented 3 years ago

Hello!

I am the maintainer of https://github.com/agersant/polaris, a self-hosted music streaming service in Rust. I am tentatively migrating the web server from rocket 0.4 (non async) + diesel + r2d2 to actix web + diesel + bb8. Thank you for writing this library which is making the process easier.

One issue I did run into, is that actix web and bb8-diesel seem to have conflicting requirements.

To play nice with an existing tokio runtime, actix web must run within a LocalSet as described here and here. However, bb8-diesel relies on tokio::task::block_in_place which cannot be called from a LocalSet. As a result, any attempt to use bb8-diesel within an actix web endpoint results in a panic.

Would you be interested in a contribution which updates bb8-diesel to use spawn_blocking instead of block_in_place? If you are interested, would you want this as optional behavior (specified how?) or as a replacement for the existing version?

Many thanks in advance!

agersant commented 3 years ago

Looking a bit closer, I think the change to make this work would be using spawn_local, and not spawn_blocking . Neither of these can be awaited on from the methods of the Connection trait because they are not async. I'm not sure this is solvable :(

Darksonn commented 3 years ago

I think the change to make this work would be using spawn_local, and not spawn_blocking

Just to be clear, these functions don't do the same thing, and you can't replace one with the other. The spawn_blocking method requires everything you pass it to be 'static, and I believe that's why block_in_place was used.

hronro commented 3 years ago

Any updates?

smklein commented 3 years ago

I took a stab at replacing the usage of block_in_place with other runtime-friendlier options a while back. However, it's so deeply ingrained within this crate - basically all methods use it - that I don't really think this is a possible change without fundamentally changing the caller's expectations. Namely, bb8-diesel does not impose a Send + 'static constraint on the queries - exactly as @Darksonn mentioned - which makes a drop-in replacement difficult.

I have created an alternative to this crate here: https://github.com/oxidecomputer/async-bb8-diesel , with a pretty full README.md file describing the pros / cons of one crate versus another. I think both solutions have advantages and disadvantages, but if you are okay with the Send + 'static constraint, but not okay with the block_in_place invocations, I'd recommend looking there.