rails/rails — Issue #5330: ConnectionPool.with_connection does not always properly release connection
Author: jrochkind
Created: 2012-03-07 18:21:50+00:00
State: open

Okay, I think I'm finally getting to the bottom of my ActiveRecord concurrency issues. Rails 3.2.2

ConnectionPool.with_connection is supposed to check out a connection assigned to the thread if one isn't already checked out; then check it back in after the yield if a new one was checked out by with_connection.

I believe it is broken. I have a demonstration Rails app I can put somewhere if someone needs it. But I guess I should really come up with a failing list, and perhaps a fix. In 3.2 stable branch?

I believe the problem is right here ( in 3.2.2 tagged snapshot of connection pool):

https://github.com/rails/rails/blob/c606fe2c6f1112d13b5ff01f3204e29cbdb09649/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb#L117

with_connection decides if it's a "fresh_connection" situation by calling active_connection? . Then it just yields the block -- if a connection wasn't already checked out to the thread, any ActiveRecord activity will automatically check one out. Then after, yield, it calls release_connection only if there was fresh_connection.

The problem is that the current implementation of active_connection? does **not** check to see if the current thread had a checked out connection. It checks to see if ANY thread had a checked out connection. https://github.com/rails/rails/blob/c606fe2c6f1112d13b5ff01f3204e29cbdb09649/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb#L100

So if some thread somewhere has an active connection, and then another thread uses #with_connection -- a new connection will be checked out for that thread, but NOT checked in at the end of with_connection.

Bug.

I think this is what's going on. I think @tenderlove may be the (only?) active committer that actually understands this stuff at all. Would GREATLY appreciate some feedback.

I can try to create a failing test case and even a patch (will probably require the introduction of a new method, #current_thread_connection? or something, since #active_connection? has the semantics it does now (it may not always have had them, but who knows if changing them now would break something else)). But I've never contributed to rails before, not sure about the structure of the tests and getting a setup that allows you to run tests and such, appreciate any pointers. Presumably I'd work off of the rails 3.2 stable branch for a patch?