djc / bb8

Full-featured async (tokio-based) postgres connection pool (like r2d2)
MIT License
769 stars 110 forks source link

PooledConnection return on panic #211

Open SafariMonkey opened 5 months ago

SafariMonkey commented 5 months ago

In PooledConnection's Drop impl, it seems that unless the connection has been extracted via take, the connection is unconditionally returned to the queue. This concerns me, as the Drop may have been invoked during unwinding, and the connection itself may have initiated the panic, and as such may contain broken invariants, which has_broken and is_valid may not be appropriate to detect.

I considered wrapping my connection type in a type that detects panics on drop, but, of course, the inner connection type does not get dropped if it's returned to the pool. It might be possible to wrap the connection type after .get()ing it from the pool in a type that extracts and drops it on drop if the thread is panicking, but that seems awkward and error-prone. (I tried the outer wrapper approach, but take() isn't public.)

Is there currently an intended way to handle panics? If there isn't, then if my concern is valid, might it be reasonable to (maybe based on a flag) check std::thread::panicking in the Drop implementation, and drop the connection instead of returning it to the pool if it's true?

djc commented 5 months ago

This concerns me, as the Drop may have been invoked during unwinding, and the connection itself may have initiated the panic, and as such may contain broken invariants, which has_broken and is_valid may not be appropriate to detect.

This seems pretty theoretical -- do you have a concrete scenario in mind?

I suppose it might not hurt too avoid calling put_back() in the panicking() case. However, in that case we should probably make sure to call replenish_idle_connections().

SafariMonkey commented 5 months ago

So I don't have a case (edit: to hand) for broken invariants within the underlying transport, but as a simple example, the current use case involves sending N messages on a WebTransport (~QUIC) stream and awaiting N messages in response. In case of a panic, there may still be messages in transit for that stream ID, so that implicit pairing of requests and responses may go out of sync. Of course, there are designs that can handle this better without closing the connection, but that's an example of a use case.