Unwind safety

[dchenk]

Would it be feasible or even make sense to implement UnwindSafe for the managed::Pool type?

Some context: I've decided to replace r2d2 with deadpool-diesel for database connection pooling in an application because r2d2 has a serious flaw in the way it handles panics (see diesel-rs/diesel#2124, diesel-rs/diesel#2105, diesel-rs/diesel#2020, sfackler/r2d2#70).

The managed::Pool type not being UnwindSafe prevents certain use cases such as passing the pool to an async function that may panic but where the panic needs to be caught. As far as I can tell, because pooled objects are wrapped in a Mutex and deadpool discards poisoned mutexes, there's no risk of reusing an invalid pooled object. (At least this is the case with the diesel and sqlite implementations.)

[bikeshedder]

As far as I understand the marker trait the core of deadpool should mostly be UnwindSafe.

I have to double check the Drop implementations, but if I'm not mistaken the following code should be fine to add:

impl<M: Manager, W: From<Object<M>>> UnwindSafe for Pool<M, W>
where
    M: UnwindSafe,
    W: UnwindSafe,
{
}

The deadpool-diesel and deadpool-sqlite crates should indeed be UnwindSafe. I'm confident deadpool-postgres and deadpool-lapin are UnwindSafe, too. Regarding deadpool-redis the underlying redis crate doesn't even support future cancellation so it's probably not UnwindSafe either.

[dchenk]

I'm seeing a type error with the managed::PoolInner::semaphore field though (https://github.com/bikeshedder/deadpool/blob/master/src/managed/mod.rs#L461):

the type `UnsafeCell<AtomicUsize>` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary

    = help: within `Pool<Manager<diesel::PgConnection>>`, the trait `RefUnwindSafe` is not implemented for `UnsafeCell<AtomicUsize>`
    = note: required because it appears within the type `tokio::loom::std::atomic_usize::AtomicUsize`
    = note: required because it appears within the type `tokio::sync::batch_semaphore::Semaphore`
    = note: required because it appears within the type `tokio::sync::semaphore::Semaphore`
    = note: required because it appears within the type `deadpool::managed::PoolInner<Manager<diesel::PgConnection>>`
    = note: required because it appears within the type `alloc::sync::ArcInner<deadpool::managed::PoolInner<Manager<diesel::PgConnection>>>`
    = note: required because it appears within the type `PhantomData<alloc::sync::ArcInner<deadpool::managed::PoolInner<Manager<diesel::PgConnection>>>>`
    = note: required because it appears within the type `Arc<deadpool::managed::PoolInner<Manager<diesel::PgConnection>>>`
    = note: required because it appears within the type `Pool<Manager<diesel::PgConnection>>`
    = note: required because of the requirements on the impl of `UnwindSafe` for `&Pool<Manager<diesel::PgConnection>>`

The same error is given for dyn deadpool::managed::hooks::PostCreate<Manager<diesel::PgConnection>> + 'static — referring to the managed::PoolInner::hooks field.

[bikeshedder]

I keep coming back to this issue and think about it.

I do wonder if an object should ever be returned to the pool if it comes from a thread that just panicked. If the object caused the panic it's very likely that it's going to happen again and therefore the object should be considered "broken beyond repair". Though since there is no way to know what caused the panic the worst should be assumed, no?

An easy fix would be to check std::thread::panicking() in the drop method of the Object and not return the object to the pool.

Thinking about it. Maybe this should added as a new configuration variable:

/// This flag controls if objects that were dropped as part of a thread
/// panicking should be returned to the pool or not.
///
/// **Note:** There is no way to decide wether the object caused the panic
/// or some user code. So it's usually a safe bet to discard the object just to
/// be sure.
return_on_panic: bool;

The default for this should be false of course.

Besides that maybe a panic hint should be added to the Object type in general which could be used to override this behavior like that:

let panic_guard = Object::panic_guard();
/// panic guard enabled. Panics from this point on will
/// not return the object to the pool.
drop(panic_guard);
/// panic guard disabled. Panics from this point on will
/// return the object as usual.

I guess I'm overthinking this. Usually code should not be built around panics and it's probably the best idea to always discard objects on panic. Just to be safe.

[bikeshedder]

A simple check of std::thread::panicking() in the Drop implementation of the object should fix all non-sync pool implementations. It would be nice if there were also tests for object creating and recycling as well making sure that the pool state can't be corrupted by misbehaving manager implementations.

[fabianfreyer]

As a side-note, this is currently blocking deadpool being used in a Gotham Middleware because of the RefUnwindSafe requirement on Gotham's NewMiddleware trait.
I've drafted up #239 to test this, but I'm still running into trouble:

error[E0277]: the type `(dyn for<'a> Fn(&'a mut ClientWrapper, &'a deadpool_postgres::Metrics) -> Pin<Box<(dyn futures::Future<Output = Result<(), deadpool::managed::hooks::HookError<tokio_postgres::Error>>> + std::marker::Send + 'a)>> + std::marker::Send + Sync + 'static)` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
   --> src/lib/http.rs:61:19
    |
61  | pub(crate) struct InjectDatabasePool<'a> {
    |                   ^^^^^^^^^^^^^^^^^^^^^^ `(dyn for<'a> Fn(&'a mut ClientWrapper, &'a deadpool_postgres::Metrics) -> Pin<Box<(dyn futures::Future<Output = Result<(), deadpool::managed::hooks::HookError<tokio_postgres::Error>>> + std::marker::Send + 'a)>> + std::marker::Send + Sync + 'static)` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
    |
    = help: within `alloc::sync::ArcInner<deadpool::managed::PoolInner<Manager>>`, the trait `RefUnwindSafe` is not implemented for `(dyn for<'a> Fn(&'a mut ClientWrapper, &'a deadpool_postgres::Metrics) -> Pin<Box<(dyn futures::Future<Output = Result<(), deadpool::managed::hooks::HookError<tokio_postgres::Error>>> + std::marker::Send + 'a)>> + std::marker::Send + Sync + 'static)`
    = note: required because it appears within the type `PhantomData<dyn Fn(&mut ClientWrapper, &Metrics) -> Pin<Box<dyn Future<Output = Result<(), HookError<Error>>> + Send>> + Send + Sync>`
    = note: required because it appears within the type `Unique<dyn Fn(&mut ClientWrapper, &Metrics) -> Pin<Box<dyn Future<Output = Result<(), HookError<Error>>> + Send>> + Send + Sync>`
    = note: required because it appears within the type `Box<dyn Fn(&mut ClientWrapper, &Metrics) -> Pin<Box<dyn Future<Output = Result<(), HookError<Error>>> + Send>> + Send + Sync>`
    = note: required because it appears within the type `Hook<Manager>`
    = note: required because it appears within the type `PhantomData<Hook<Manager>>`
    = note: required because it appears within the type `Unique<Hook<Manager>>`
    = note: required because it appears within the type `RawVec<Hook<Manager>>`
    = note: required because it appears within the type `Vec<Hook<Manager>>`
    = note: required because it appears within the type `HookVec<Manager>`
    = note: required because it appears within the type `Hooks<Manager>`
    = note: required because it appears within the type `PoolInner<Manager>`
    = note: required because it appears within the type `ArcInner<PoolInner<Manager>>`
    = note: required for `NonNull<alloc::sync::ArcInner<deadpool::managed::PoolInner<Manager>>>` to implement `UnwindSafe`
    = note: required because it appears within the type `Weak<PoolInner<Manager>>`
    = note: required because it appears within the type `Object<Manager>`
    = note: required for `deadpool::managed::Pool<Manager>` to implement `RefUnwindSafe`
    = note: required because it appears within the type `&Pool<Manager>`
note: required because it appears within the type `InjectDatabasePool<'a>`
   --> src/lib/http.rs:61:19
    |
61  | pub(crate) struct InjectDatabasePool<'a> {
    |                   ^^^^^^^^^^^^^^^^^^
note: required by a bound in `NewMiddleware`
   --> /.cargo/registry/src/github.com-1ecc6299db9ec823/gotham-0.7.1/src/middleware/mod.rs:366:33
    |
366 | pub trait NewMiddleware: Sync + RefUnwindSafe {
    |                                 ^^^^^^^^^^^^^ required by this bound in `NewMiddleware`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `moodle-eventqueue-processor` due to 10 previous errors

In the implementation in #139 (comment), I'm not sure I understand why the UnwindSafe bound on W is required, should this not suffice, since W is only used in a PhantomData?

impl<M: Manager, W: From<Object<M>>> UnwindSafe for Pool<M, W>
where
    M: UnwindSafe,
{
}