Add unwind safety #70
Conversation
Currently `Pool` does not implement `UnwindSafe` or `RefUnwindSafe`. This is due to `Condvar` not implementing it in the standard library. That type probably should implement it, but `Pool` shouldn't be `UnwindSafe` prior to this commit anyway. `antidote::Mutex` incorrectly implements `UnwindSafe`, when it should not as it removes the poisoning mechanism that makes `Mutex` be `UnwindSafe` in the first place. Ultimately, prior to this commit, `Pool<M>` should only be `UnwindSafe` if `M: UnwindSafe` and `M::Connection: UnwindSafe`. The need for that bound on `M::Connection` is because we return the connection to the pool on panic, even if it's in a potentially invalid state. This commit adds explicit implementations of `UnwindSafe` and `RefUnwindSafe`, and also removes the need to bound that on the connection being `UnwindSafe` by only returning a connection to the pool if it was not dropped during a panic. This ensures that we don't end up in a situation where a connection is potentially returned to the pool in a state where `is_broken` would return `true`, but it is not in an expected state (e.g. having an open transaction). It also means that the connection can be expected to be dropped if a panic occurs while it is being used (e.g. ensuring the connection is terminated if there was an open transaction). We still need an `UnwindSafe` bound on the connection manager, as we can't guarantee that it will be in a reasonable internal state if a panic occurs in one of its methods Fixes sfackler#63 Fixes sfackler#31
src/lib.rs
Outdated
| self.pool.put_back(self.conn.take().unwrap()); | ||
| use std::thread::panicking; | ||
|
|
||
| if panicking() { |
There was a problem hiding this comment.
I'd prefer this be controlled by a configuration flag.
There was a problem hiding this comment.
How would you feel about having this controlled by a cargo feature instead? The issue is that we need a M::Connection: UnwindSafe bound on the impls of UnwindSafe and RefUnwindSafe if we don't do this, so having it controlled by a runtime config flag means we either can't provide those impls, or providing them is potentially unsafe.
There was a problem hiding this comment.
What's the problem with adding that extra bound?
There was a problem hiding this comment.
We don't expect it to be true the majority of the time. It is not true for postgres::Connection or any of the connection types in Diesel.
We're exposing references to these types and storing them internally. If those types always get returned to the pool on drop, then we're only UnwindSafe if those types are. All these types use interior mutability, which is already opts them out of this trait.
So we can only be considered UnwindSafe if we are either not returning connections to the pool on panic, or the connections being returned are also UnwindSafe (and probably RefUnwindSafe on both impls). If this is configurable at runtime, we have to assume it's not safe to implement this. If we controlled it with a cargo feature instead, we could cfg out the impls.
|
At a philosophical level, I have a couple of concerns: More broadly than this single issue, I think The fact that In this specific instance, I don't think that dropping all connections returned while a thread is panicking is actually the right thing to do. "Something bad happened, so I guess we'll just throw this resource away just in case it was somehow involved" doesn't seem right to me. The unclosed transaction example brought up in the associated issue just seems like a poor transaction API, and there are multiple potential solutions. You could use an RAII-based API to manage the transaction and roll back in the destructor. This would fix both the panic-in-transaction case, and the early-return-in-transaction case from e.g. a SQL error. We could add I'm open to making this behavior part of the configuration because it seems totally plausible to allow people to opt-into being a bit conservative in this regard. Making it a compile time Cargo feature seems like it's just going to add more complexity than it's worth. |
This changes how we handle unwind safety by adding another useful feature, public API to permanently remove a connection from the pool. When a connection is removed, the pool will immediately treat it the same as getting back a broken connection. It will decrement its active connection count, and immediately establish a new one in the pool if this brings us below min_idle. This feature allows us to prevent connections from being returned on panic by adding a new hook to `ManageConnection`, which takes `&mut PooledConnection`. It's assumed that any impl of `ManageConnection` which implements `UnwindSafe` or `RefUnwindSafe` will override this hook to restore any potentially broken invariants, or call `remove_from_pool` if it can't. We've introduced a new hook for this because we need to guarantee that it will be called from the same thread the connection has dropped from, which we don't guarantee for `has_broken` (and using that for this purpose would be confusing) I've structured things the way they are, because I want to eventually have us start assuming connections will never be returned if they're checked out for too long, so we'll need to pass the `Conn` around for book keeping
|
@sfackler I've taken another attempt at this which I think you'll find more palatable. Another feature I've been meaning to propose is adding public API to remove a connection from the pool. With that API in place, I've added a hook to |
| @@ -449,13 +479,21 @@ where | |||
| } | |||
| } | |||
|
|
|||
| fn put_back(&self, mut conn: Conn<M::Connection>) { | |||
| fn put_back(&self, conn: &mut PooledConnection<M>) { | |||
| self.0.manager.before_return(conn); | |||
There was a problem hiding this comment.
Maybe this should only be called if the connection is not broken (e.g. before line 498) so that the caller does not need to check for a broken connection as well
|
@sfackler Any thoughts on this implementation? If you're good with moving forward on this I will rebase |
Currently
Pooldoes not implementUnwindSafeorRefUnwindSafe.This is due to
Condvarnot implementing it in the standard library.That type probably should implement it, but
Poolshouldn't beUnwindSafeprior to this commit anyway.antidote::Mutexincorrectlyimplements
UnwindSafe, when it should not as it removes the poisoningmechanism that makes
MutexbeUnwindSafein the first place.Ultimately, prior to this commit,
Pool<M>should only beUnwindSafeif
M: UnwindSafeandM::Connection: UnwindSafe. The need for thatbound on
M::Connectionis because we return the connection to the poolon panic, even if it's in a potentially invalid state.
This commit adds explicit implementations of
UnwindSafeandRefUnwindSafe, and also removes the need to bound that on theconnection being
UnwindSafeby only returning a connection to the poolif it was not dropped during a panic. This ensures that we don't end up
in a situation where a connection is potentially returned to the pool in
a state where
is_brokenwould returntrue, but it is not in anexpected state (e.g. having an open transaction). It also means that the
connection can be expected to be dropped if a panic occurs while it is
being used (e.g. ensuring the connection is terminated if there was an
open transaction).
We still need an
UnwindSafebound on the connection manager, as wecan't guarantee that it will be in a reasonable internal state if a
panic occurs in one of its methods
Fixes #63
Fixes #31