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

Already on GitHub? Sign in to your account

QueryCache/ConnectionPool thread fix #1670

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
6 participants
Contributor

mjtko commented Jun 13, 2011

Under thin, QueryCache is leaking connections.

This is caused by the fact that the thread that runs the QueryCache::BodyProxy#close method is not the same as the thread that executes QueryCache#call. A connection is checked out within QueryCache#call; this connection becomes orphaned, while a new connection is checked out within #close when theclear_query_cache call is made.

This patch prevents the connection checked out within QueryCache#call from being orphaned by creating an identifier that is stored within a thread local and used within ConnectionPool to uniquely identify the connection. This identifier is passed to BodyProxy at construction time and pushed into the thread that ultimately becomes responsible for the clear_query_cache call.

The connection is subsequently checked back in as expected at the end of the request lifecycle, plugging the leak.

Thanks for patching this. Hopefully someday this will be merged into 3.1.something and I can run multi-threaded apps under Thin again. It's killing me.

Owner

spastorino commented Oct 6, 2011

Hey @mjtko can you rebase your patch?

Contributor

mjtko commented Oct 6, 2011

Hi,

Doubt this will make it into core... It's not exactly a pleasant our elegant hack! :-)

Perhaps a monkey patch gem of some kind for thin though?

PS. Personally, I'm using Rainbows! and DataMapper these days. :-)

Cheers,

Mark.

[Sent from a mobile device; please excuse my brevity and top reply.]

Mark J. Titorenko

Jordan Hollinger reply@reply.github.com wrote:

Thanks for patching this. Hopefully someday this will be merged into 3.1.something and I can run multi-threaded apps under Thin again. It's killing me.

Reply to this email directly or view it on GitHub:
#1670 (comment)

I don't know, I'm sure there's worse code in Rails :-)

I suppose the question is "has Thin been doing things wrong all along, and it just now became obvious?", in which case Thin should be patched, or "has Rails 3.1 accidentally broken a sane expectation held by Thin?", in which case Rails should be patched. Honestly I don't have enough knowledge of either code base to hazard a guess.

Contributor

mjtko commented Oct 6, 2011

@spastorino I'll rebase it tomorrow (Europe), though, personally, I think this should wait until after 3.1.1 so, even though I'll rebase it against 3-1-stable, I'd wait until after 3.1.1 is out of the door before pulling. Up to you to decide of course! :)

@jhollinger This has always been the case when running Rails in threadsafe mode under Thin (AFAICS) and, while from my POV Thin is doing the right thing, Rails has been making an assumption about the threading model within a rack server - I would expect Rainbows! with the event machine concurrency model to suffer the same problem.

In short, my belief is that active record should be patched to support this use case.

Owner

spastorino commented Oct 7, 2011

@mjtko don't worry this is not going in 3.1.1. Btw please rebase it on top of master, we will merge in 3-1-stable. Thanks.

mjtko added some commits Jun 13, 2011

use thread locals and an instance variable within QueryCache#BodyProx…
…y to maintain appropriate linkage with AR database connection across threads
Merge branch 'connection-pool-thread-fixes' of github.com:mjtko/rails…
… into connection-pool-thread-fixes

Conflicts:
	activerecord/lib/active_record/query_cache.rb
Contributor

mjtko commented Oct 7, 2011

Argh, I've totally messed this branch up trying to rebase it! Rather than trying to unpick the mess I'll open a new pull request against master. :)

@mjtko mjtko closed this Oct 7, 2011

Contributor

mjtko commented Oct 7, 2011

@spastorino Opened against master as #3243. Sorry for the confusion. :-)

Contributor

artemave commented Oct 7, 2011

+1

very annoying issue

spastorino added a commit that referenced this pull request Oct 7, 2011

Merge pull request #3243 from mjtko/connection-pool-thread-fix
QueryCache/ConnectionPool thread fix (was #1670)

spastorino added a commit that referenced this pull request Oct 7, 2011

Merge pull request #3243 from mjtko/connection-pool-thread-fix
QueryCache/ConnectionPool thread fix (was #1670)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment