Sqlx: Pool can "leak" connections under certain conditions

[ianthetechie]

First off, thanks so much for open-sourcing this crate! This comes at a great time as the community begins to adopt async/await, so it's refreshing to see a crate that's natively supporting this as well as promoting safety at all levels! This is especially timely as there is no de facto async database connection pooling crate, and the future of the closest thing (bb8) seems to be uncertain.

The pool implementation is quite clever in some points, such as the way Option::take() is used, but under certain conditions, namely future cancellation, certain assumptions in the implementation seem to break down, causing "leaks." I use quotes because in some cases, there is not necessarily a resource leak, but the internal counter is not updated properly so the pool will refuse to allocate new connections, resulting in a sort of resource deadlock scenario.

The core issue seems to be failure to account for future cancellation. Specifically, it seems to break most obviously when under heavy resource contention (there are a lot of waiters) and a number of futures are abruptly cancelled in-between pieces of a critical section during acquire(). While this problem isn't unique to the async/await syntax, it makes it easier to forget about handling this class of error. While you are generally guaranteed that a block of code will execute to completion once it is started, async fns change these assumptions as each .await introduces a yield point, after which execution is no longer guaranteed to continue. This is the core of the problem as far as I can tell. For more reading on the subject, these links have been quite helpful: https://internals.rust-lang.org/t/async-await-the-challenges-besides-syntax-cancellation/10287/21, https://internals.rust-lang.org/t/asynchronous-destructors/11127.

Getting a bit more into the weeds, futures are cancelled by simply being dropped. This can happen in, say, the case where a user cancels their request to a web server, and part of the request never finishes processing. In the crate, there are a few types of interest to the discussion: Live<C>, Idle<C>, and PoolConnection<C>. The latter implements the Drop trait, which ensures that it is cleaned up by the pool when it goes out of scope in user-facing code. This is a great application of the RAII principle. The other two types are used internally. They do not implement Drop, which can cause some problems. These are used in problematic ways in the inner acquire() function, as they do not account for .await yield points.

The most obvious failure mode triggered by my load test scenario seems to be related to waiters. If a PoolConnection<C> is dropped, the Live<C> is unwrapped from the option, and passed to the pool release method. In the case that a waiter exists, the Live<C> is pushed to the waiter. If it succeeds, the instance will end up inside a branch of the acquire() method. This however has at least two potential failure points: 1) the future being dropped during the call to check_live, and 2) the future being dropped before its value can be mapped into a PoolConnection<C> by the calling acquire() in the public interface.

Additionally, it seems possible that the pool counter could be incremented before the Live<C> is ever wrapped in a PoolConnection<C>. In fact, the connect() could fail completely and the counter would still get incremented, so you would quickly end up in a state where a network glitch renders a server unresponsive.

I have spent the last few days digging into this, and I have some (rather hackish for the moment) solutions to some of the issues, but my feeling is that it will be very difficult to create a robust pool while maintaining such primitive, error-prone counter logic. I think a more rustic approach would be to have the pool own the connections, rather than the other way around, but I wanted to get your feedback before continuing any further. I presume there was some thought behind this decision and I'd love to hear your feedback on this and other approaches to solving this issue.

[abonander]

Futures cancellation is admittedly an area which we haven't considered very carefully.

I see where SharedPool::size can fail to be decremented if the acquire() future is dropped. I believe we can address this with a Drop guard in self.connect().

[ianthetechie]

Thanks for the quick response!

Are you referring to this crate? https://docs.rs/drop_guard/0.2.1/drop_guard/

I think that would be a quick route to solving the obvious issues, but I'm still a bit worried about the brittleness of the approach. It puts a lot of responsibility on authors / maintainers to maintain invariants, much like doing manual memory management.

What our thoughts on having the pool own the connection wrappers (in all variations) rather than the other way around? This would probably let you get rid of counters altogether and just have an internal list of live in addition to the idle connections. This still wouldn't be full RAII, but it's a huge step closer, and would make maintenance a fair bit easier in my opinion.

[abonander]

I was thinking a wrapper for AtomicUsize that better codified the acquire/release mechanics, probably in a submodule so the internal count can't be modified except through the API. That would make the count management significantly more robust.

[mehcode]

What our thoughts on having the pool own the connection wrappers

The pool cannot own the connections and have the task that checks out the connection mutably write to the TCP stream without involving a background task which is a significant drop to performance from earlier profiling.

[abonander]

And I believe prevent us from implementing the Transactions API how we want to.

[mehcode]

@ianthetechie I don't have a lot of time to discuss this right now but I want to say thank you for opening the issue. I would love :heartbeat: integration tests for the pool that stress it and break its invariants. I would merge a failing test like that in a heartbeat and then we can iterate on a way to fix this reliably while maintaining the ownership transfer which I believe is critical to the performance and API design.

[ianthetechie]

Sure thing. I'll try to work up a minimum failing test. I discovered this by load testing a simple hyper service with wrk, which ends up killing / leaving hanging connections at the end of each run, but that's not a very easily reproducible test, so I'll try to work up something a bit more compact for you guys.

Thanks a ton for your responsiveness!

[mehcode]

@ianthetechie Sanity check. Are you using 0.2+ of the crate?

---

I primarily use siege for my load testing but there was an issue with high load in the pools that I fixed in 0.2.

[ianthetechie]

Good question. I've reproduced on 0.2.4 and master (basically the same now haha). I actually have a sort of working fix locally, but as I said, it needs some serious clean-up and I want to make sure you guys are cool with the direction before going into it much more.

The pool does hold up under load (say, running wrk for 5 mins). The issue happens at the transition points (when you kill a huge test). Each time this happens, it "loses" a few more connections and eventually after 3 or so tests with my current settings it will be permanently exhausted due to counter issues.

[mehcode]

Thanks for clarifying. :+1:

[mehcode]

@ianthetechie We think this has been resolved in v0.2.5. When you get time, please give it a try. From our testing it certainly makes the Pool stable under the pathological conditions we were able to throw at it.

[ianthetechie]

Thanks! I will check it out this weekend! Sorry I haven’t gotten around to
investigating whether hyper may be at fault. It’s a thought that crossed my
mind, but I haven’t had the chance to do a deep dive yet.

2020년 2월 1일 (토) 17:56, Ryan Leckey [e-mail]님이 작성:

@ianthetechie https://github.com/ianthetechie We think this has been
resolved in v0.2.5. When you get time, please give it a try. From our
testing it certainly makes the Pool stable under the pathological
conditions we were able to throw at it.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/launchbadge/sqlx/issues/83?email_source=notifications&email_token=AAQKSGENXJQNA5PMFDYA2ZTRAU2K3A5CNFSM4KKBJDI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKQYBLI#issuecomment-581009581,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAQKSGBM376AX2PRD56YI5TRAU2K3ANCNFSM4KKBJDIQ
.

[ianthetechie]

Confirmed working fix. Thanks guys!

[singggum3b]

Hi guys.
I'm learning rust and using sqlx for my db layer.

I think the pool connection leak issue still not fixed.
I'm using warp, hyper, and sqlx together (on sqlite), and while benchmarking with wrk i noticed that whenever hyper thrown up on connection error, the future get cancelled and sqlx pool waiter is not released correctly.

Cargo.toml

[dependencies]
async-std = { version = "1", features = ["attributes", "tokio02"]}
serde = { version = "1", features=["derive"] }
serde_json = "1"
chrono = { version = "0.4", features = ["serde"] }
tokio = { version = "0.2", features = ["full"] }
warp = "0.2"
pretty_env_logger = "0.3"
hyper = "0.13.6"
shaku = "0.5"
trilogy = { path = "trilogy", version = "0.1.0"}
async-trait = "^0.1.36"
sqlx = { version = "0.4.0-beta.1", features = ["sqlite",  "chrono"] }
uuid = { version = "0.8", features = ["serde", "v4"] }
log = "0.4.0"
env_logger = "0.7.1"

The warp filter look like this:

Hyper error on highload:
DEBUG hyper::server::conn::spawn_all] connection error: connection closed before message completed

and then:

I saw drop trait is implemented for PoolConnection but somehow the waiter is not released properly ?

[singggum3b]

@abonander @mehcode @ianthetechie

[singggum3b]

acquire function might be a source of leak. When calling acquire on a pool and waiter is pushed to the list, but then the acquire future is then dropped before the timeout, then the waiter is stuck in the list indefinitely.