rails/rails — pull request #5344 by jrochkind (2012-03-08 22:53:35+00:00)
Title: ConnectionPool#with_connection properly releases connection when needed. Re #5330

@tenderlove, appreciate any feedback.

If I've done the pull request wrong somehow, let me know (i had an epic battle with git to try and get this right).

This pull request to 3-2-stable branch, hopefully that's right. I didn't want to deal with figuring out if things had changed in master yet regarding connection pools, it took me long enough to understand what was going on in 3.2.

If you have anything you would like changed in the code or test, let me know and I'll do it if I can.

The test isn't as non-fragile and clean as I'd like, but it's as clean as I could figure out to make it based on existing ConnectionPool API -- there are other existing tests that call instance_variable_get(:@reserved_connections) like this too.

The code is the smallest change I could make, without actually refactoring or changing semantics of any existing methods. I was scared to do that. It is true that the term "active connection" is used in two incompatible ways in existing code base (does it mean "the connection for current thread" or does it mean "any currently checked-out connection"? method/variable names/comments are not consistent. ) But I don't have the courage to try to refactor this stuff more -- it's fairly untested code, and concurrency is always confusing, and who knows what existing third party downstream dependencies may rely on public methods staying as they are. So I changed as little as possible to fix the problem.

Included test runs failing where expected before included change (and with expected deprecation warnings printed out about 'close your own connection'), and runs passing after included change (without any deprecation warnings).

thanks for your help/attention!