Unwinding can result in connections with broken invariants being returned to the connection pool #31
Comments
sgrif
changed the title
|
Is this in reference to a connection implementation that will be in a bad state during a panic, or is this more of a general principle of the thing? |
|
I don't have a concrete example of a connection which has broken invariants when a panic occurs if that's what you mean. I'm sure I can find one pretty easily if you really don't believe that those cases exist. |
|
The fact that I'm not aware of any instance in which this has been a problem in the last several years guides my instincts to some extent 😃 . That being said, I don't think I have a problem dropping connections that are checked back in after a panic. It can always be made configurable if that impacts someone's use case negatively. |
|
|
I don't see how that's really related? Other kinds of "panic safety" like mutex poisoning has been around for quite a long time. |
|
I just ran into this issue - it's definitely not theoretical, and has some pretty nasty consequences: namely transactions may not be closed correctly when a thread panics. Since database locks are held until the transaction is closed, this can result in the entire system locking up indefinitely. I believe the "most correct" solution here would be to require manually returning the connection to the pool, because using For backwards compatibility, perhaps you could add a "discard" method and an "unwind-safe" wrapper which automatically discards the connection when dropped unless you explicitly return it to the pool. |
|
Hm, on second thoughts, the |
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). Fixes sfackler#63 Fixes sfackler#31
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
|
We're running into this issue in production. Does anyone know a workaround that works now? |
|
Can you not use RAII for your transaction management? |
|
@sfackler You mean rolling back the transaction on Drop if the drop was caused by a panic? |
|
I mean rolling back the connection on Drop if the transaction hasn't been committed: https://github.com/sfackler/rust-postgres/blob/master/tokio-postgres/src/transaction.rs#L30 |
Currently
PooledConnectionunconditionally returns the connection to the pool on drop. Since a panic could happen at any time, we can't assume that the connection will be in a good state if it's being dropped as the result of unwinding. r2d2 should either checkthread::panickingbefore checking the connection back into the pool, or should have aT: UnwindSafeconstraint on the connection.The text was updated successfully, but these errors were encountered: