Pool should implement RefUnwindSafe
#63
Comments
sgrif
added a commit
to sgrif/r2d2
that referenced
this issue
Jan 24, 2019
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
sgrif
added a commit
to sgrif/r2d2
that referenced
this issue
Jan 24, 2019
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
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
All types which are
Syncshould also implementRefUnwindSafeas the former implies the latter. Currently thePooltype is notRefUnwindSafe(through no fault of its own) because it stores aCondvar, andCondvaris also (incorrectly) missing aRefUnwindSafeimplementation in the rust standard library.However,
r2d2could explicitly provide this implementation.The text was updated successfully, but these errors were encountered: