This project is archived and is in readonly mode.

#5736 ✓committed
luis.lopez (at branelabs)

connections not released in rails 3

Reported by luis.lopez (at branelabs) | September 29th, 2010 @ 09:27 PM

When a activerecord asks for a connection in a thread, this connections is not released after thread end. So, the connection pool can be exhausted. Definining a pool of 5 for the DB connection (mysql), this code fails after the 5th try:

puts "ruby vesion: #{RUBY_VERSION} rails version: #{Rails.version}"
1.upto(10) do |i|
     Thread.new() do
        puts "iteration #{i}"
        User.first
        puts "iteration #{i} after query"
        # ActiveRecord::Base.connection_pool.release_connection
    end.join
    # ActiveRecord::Base.connection_pool.clear_stale_cached_connections!() 
end

the result is:

$rails runner /tmp/test2.rb
ruby vesion: 1.9.2 rails version: 3.0.0
iteration 1
iteration 1 after query
iteration 2
iteration 2 after query
iteration 3
iteration 3 after query
iteration 4
iteration 4 after query
iteration 5
iteration 5 after query
iteration 6
/tmp/test2.rb:4:in join': deadlock detected (fatal)</code>
</pre>



Uncommenting one of the commented lines that manually forces to release connections makes it works but I guess it's not the expected behaviour since it worked without problems in previous version (tested with ruby 1.8.7 and rails 2.3.5)



It fails too with rails 3.1.0.beta.

Comments and changes to this ticket

  • Hemant Kumar

    Hemant Kumar September 30th, 2010 @ 10:08 AM

    • Assigned user set to “Aaron Patterson”

    Verified on Ruby 1.9. The problem with checkout methods is, it relies on return value of wait method to reclaim connections from dead threads.

    But return value of wait has been changed and in 1.9 it will return always true, whether it returns after timeout period or it returns because it was signalled by singal. In fact, return value of wait can't be relied upon and I saw the code of Rubinius and JRuby implementations as well and behaviour is not analogous to 1.8.

    Attached patch fixes the problem and it no longer depends on return value of wait method. I have also added some missing tests. One more thing that I did ended up doing was setting timeout value on 1.9; original code had timeout set to nil in Ruby 1.9. Calling wait with nil parameter can trigger deadlock as OP got in original bug report.

  • Hemant Kumar

    Hemant Kumar September 30th, 2010 @ 10:19 AM

    • Tag changed from “rails 3.0.0, connection, pool, thread” to “rails 3.0.0, connection, patched, pool, thread”
  • luis.lopez (at branelabs)

    luis.lopez (at branelabs) September 30th, 2010 @ 12:06 PM

    • Tag cleared.

    The patch works. The only drawback is the waiting time to release connections. If you run the above code, it will freeze for 5 seconds in the 6th try. If you do the test more times (1.upto(20), for instance) it will freeze 5 seconds every 5 tries (since the pool is 5 for this test). Not sure if it is possible to check if there are connections to be reclaimed before wait.

  • gnufied

    gnufied September 30th, 2010 @ 02:07 PM

    Well the patch does not modify original behaviour, it just makes it work on Ruby 1.9. One can of course check for connections held by dead threads before calling wait,but that would mean that we will have to call clear_stale_cached_connections twice:

    clear_stale_cached_connections!

if(@checked_out.size < @connections.size)
  next
else
  @queue.wait(@timeout)
end            
clear_stale_cached_connections!
if @size == @checked_out.size
  raise ConnectionTimeoutError, "could not obtain a database connection#{" within #{@timeout} seconds" if @timeout}.  The max pool size is currently #{@size}; consider increasing it."
end
  • luis.lopez (at branelabs)

    luis.lopez (at branelabs) September 30th, 2010 @ 05:18 PM

    I personally think that it would be a better solution: the best moment to check for connections is probably before the wait and I don't think that call clear_stale_cached_connections twice would be big problem since there is a wait anyway. In the worst case there will be 2 calls to clear_stale_cached_connections (one more then in the current code) and in the best the wait would be avoided.

    However, I don't know if clear_stale_cached_connections! could have a big performance penalty in case the pool were big

  • Repository

    Repository October 6th, 2010 @ 09:43 PM

    • State changed from “new” to “committed”
  • Repository

    Repository October 6th, 2010 @ 09:46 PM

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

People watching this ticket

Attachments

Referenced by

Pages