Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PooledConnection return on panic #211

Open
SafariMonkey opened this issue Jun 21, 2024 · 2 comments
Open

PooledConnection return on panic #211

SafariMonkey opened this issue Jun 21, 2024 · 2 comments

Comments

@SafariMonkey
Copy link

SafariMonkey commented Jun 21, 2024

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
Copy link
Owner

djc commented Jun 21, 2024

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
Copy link
Author

SafariMonkey commented Jun 21, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants