Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

ConnectionPool wait_timeout no longer used for different types of timeouts. #6441 #6463

Merged
merged 1 commit into from

3 participants

@jrochkind

An AR ConnectionSpec wait_timeout is pre-patch used for three
different things:

  • mysql2 uses it for MySQL's own wait_timeout (how long MySQL should allow an idle connection before closing it), and defaults to 2592000 seconds.
  • ConnectionPool uses it for "number of seconds to block and wait for a connection before giving up and raising a timeout error", default 5 seconds.
  • ConnectionPool uses it for the Reaper, for deciding if a 'dead' connection can be reaped. Default 5 seconds.

Previously, if you want to change these from defaults, you need
to change them all together. This is problematic especially
for the mysql2/ConnectionPool conflict, you will generally not
want them to be the same, as evidenced by their wildly different
defaults. This has caused real problems for people #6441 #2894

But as long as we're changing this, forcing renaming the
ConnectionPool key to be more specific, it made sense
to seperate the two ConnectionPool uses too -- these two
types of ConnectionPool timeouts ought to be able to be
changed independently, you won't neccesarily want them
to be the same, even though the defaults are (currently)
the same.

@jrochkind jrochkind ConnectionPool wait_timeout no longer used for different types of tim…
…eouts. #6441

An AR ConnectionSpec `wait_timeout` is pre-patch used for three
different things:

* mysql2 uses it for MySQL's own wait_timeout (how long MySQL
  should allow an idle connection before closing it), and
  defaults to 2592000 seconds.
* ConnectionPool uses it for "number of seconds to block and
  wait for a connection before giving up and raising a timeout error",
  default 5 seconds.
* ConnectionPool uses it for the Reaper, for deciding if a 'dead'
  connection can be reaped. Default 5 seconds.

Previously, if you want to change these from defaults, you need
to change them all together. This is problematic _especially_
for the mysql2/ConnectionPool conflict, you will generally _not_
want them to be the same, as evidenced by their wildly different
defaults. This has caused real problems for people #6441 #2894

But as long as we're changing this, forcing renaming the
ConnectionPool key to be more specific, it made sense
to seperate the two ConnectionPool uses too -- these two
types of ConnectionPool timeouts ought to be able to be
changed independently, you won't neccesarily want them
to be the same, even though the defaults are (currently)
the same.
cb6f839
@tenderlove tenderlove merged commit 43893f3 into rails:master
@rafaelfranca

@tenderlove this can be backported to 3-2-stable? I is supposed to be a bug fix to #6441, but since it is changing the public API I think it can't be backported.

@tenderlove
Owner

I don't think this patch could be backported (since the connection pool has changed so much). It looks like #6441 is specific to mysql2, so we should probably add something like this to 3-2-stable.

@jrochkind

@rafaelfranca yeah, that was my concern, changing the api. There's no way to fix this without changing the api for something. But it might be 'safer' (although more confusing) to change the api for mysql2 instead of ConnectionPool -- but it's nto entirely clear to me.

Nobody using rails 3.2.x as is, with mysql2, could possibly be setting the ConnectionPool timeout to a reasonable value without messing up their app, it wouldn't work. On the other hand, they might be using wait_timeout intending it to apply to mysql2 (a very high value), and if not doing multi-threading not noticing that that's a much higher value than they'd want for ConnectionPool's timeout.

So I'm not really sure what can/should be done in 3-2-stable. And the fact that it seems unlikely there will be another 3.2.x release incorporating any fixes lowers my motivation to spend much time on it.

But if anyone makes the call on what should happen and thinks it's worth doing, I'll prepare a backport pull for 3-2-stable (woudln't have to involve the Reaper, which doesn't exist in 3-2-stable).

Personally, I'll probably be monkey patching in 3.2.3, but I dont' need to worry about anyone elses's backwards compat but my own.

@jrochkind

@tenderlove the main problem is all specific to mysql2 -- the conflict between mysql2's wait_timeout and ConnectionPool's was the main problem, which existed in 3-2-stable and master. That was the real problem which was horrible, the reaper/checkout_timeout was something subsidiary it just made sense to clean up as long as it was being changed.

If someone decides what the "right" behavior in 3-2-stable is, I can prepare a backported pull. Would probably do it from scratch rather than try to cherry-pick, since the implemetnation has changed enough. But it's not at all clear to me what the right thing to do in 3-2-stable is, with backwards compat concerns.

@tenderlove
Owner

@jrochkind OK, let's move this conversation to #6441.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 23, 2012
  1. @jrochkind

    ConnectionPool wait_timeout no longer used for different types of tim…

    jrochkind authored
    …eouts. #6441
    
    An AR ConnectionSpec `wait_timeout` is pre-patch used for three
    different things:
    
    * mysql2 uses it for MySQL's own wait_timeout (how long MySQL
      should allow an idle connection before closing it), and
      defaults to 2592000 seconds.
    * ConnectionPool uses it for "number of seconds to block and
      wait for a connection before giving up and raising a timeout error",
      default 5 seconds.
    * ConnectionPool uses it for the Reaper, for deciding if a 'dead'
      connection can be reaped. Default 5 seconds.
    
    Previously, if you want to change these from defaults, you need
    to change them all together. This is problematic _especially_
    for the mysql2/ConnectionPool conflict, you will generally _not_
    want them to be the same, as evidenced by their wildly different
    defaults. This has caused real problems for people #6441 #2894
    
    But as long as we're changing this, forcing renaming the
    ConnectionPool key to be more specific, it made sense
    to seperate the two ConnectionPool uses too -- these two
    types of ConnectionPool timeouts ought to be able to be
    changed independently, you won't neccesarily want them
    to be the same, even though the defaults are (currently)
    the same.
This page is out of date. Refresh to see the latest.
View
23 activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
@@ -55,19 +55,27 @@ module ConnectionAdapters
#
# == Options
#
- # There are two connection-pooling-related options that you can add to
+ # There are several connection-pooling-related options that you can add to
# your database connection configuration:
#
# * +pool+: number indicating size of connection pool (default 5)
- # * +wait_timeout+: number of seconds to block and wait for a connection
+ # * +checkout_timeout+: number of seconds to block and wait for a connection
# before giving up and raising a timeout error (default 5 seconds).
+ # * +reaping_frequency+: frequency in seconds to periodically run the
+ # Reaper, which attempts to find and close dead connections, which can
+ # occur if a programmer forgets to close a connection at the end of a
+ # thread or a thread dies unexpectedly. (Default nil, which means don't
+ # run the Reaper).
+ # * +dead_connection_timeout+: number of seconds from last checkout
+ # after which the Reaper will consider a connection reapable. (default
+ # 5 seconds).
class ConnectionPool
# Every +frequency+ seconds, the reaper will call +reap+ on +pool+.
# A reaper instantiated with a nil frequency will never reap the
# connection pool.
#
# Configure the frequency by setting "reaping_frequency" in your
- # database yaml file.
+ # database yaml file.
class Reaper
attr_reader :pool, :frequency
@@ -89,7 +97,7 @@ def run
include MonitorMixin
- attr_accessor :automatic_reconnect, :timeout
+ attr_accessor :automatic_reconnect, :checkout_timeout, :dead_connection_timeout
attr_reader :spec, :connections, :size, :reaper
class Latch # :nodoc:
@@ -121,7 +129,8 @@ def initialize(spec)
# The cache of reserved connections mapped to threads
@reserved_connections = {}
- @timeout = spec.config[:wait_timeout] || 5
+ @checkout_timeout = spec.config[:checkout_timeout] || 5
+ @dead_connection_timeout = spec.config[:dead_connection_timeout]
@reaper = Reaper.new self, spec.config[:reaping_frequency]
@reaper.run
@@ -241,7 +250,7 @@ def checkout
return checkout_and_verify(conn) if conn
end
- Timeout.timeout(@timeout, PoolFullError) { @latch.await }
+ Timeout.timeout(@checkout_timeout, PoolFullError) { @latch.await }
end
end
@@ -279,7 +288,7 @@ def remove(conn)
# or a thread dies unexpectedly.
def reap
synchronize do
- stale = Time.now - @timeout
+ stale = Time.now - @dead_connection_timeout
connections.dup.each do |conn|
remove conn if conn.in_use? && stale > conn.last_use && !conn.active?
end
View
4 activerecord/test/cases/connection_pool_test.rb
@@ -124,7 +124,7 @@ def test_reap_and_active
@pool.checkout
@pool.checkout
@pool.checkout
- @pool.timeout = 0
+ @pool.dead_connection_timeout = 0
connections = @pool.connections.dup
@@ -137,7 +137,7 @@ def test_reap_inactive
@pool.checkout
@pool.checkout
@pool.checkout
- @pool.timeout = 0
+ @pool.dead_connection_timeout = 0
connections = @pool.connections.dup
connections.each do |conn|
View
4 activerecord/test/cases/pooled_connections_test.rb
@@ -17,7 +17,7 @@ def teardown
end
def checkout_connections
- ActiveRecord::Model.establish_connection(@connection.merge({:pool => 2, :wait_timeout => 0.3}))
+ ActiveRecord::Model.establish_connection(@connection.merge({:pool => 2, :checkout_timeout => 0.3}))
@connections = []
@timed_out = 0
@@ -34,7 +34,7 @@ def checkout_connections
# Will deadlock due to lack of Monitor timeouts in 1.9
def checkout_checkin_connections(pool_size, threads)
- ActiveRecord::Model.establish_connection(@connection.merge({:pool => pool_size, :wait_timeout => 0.5}))
+ ActiveRecord::Model.establish_connection(@connection.merge({:pool => pool_size, :checkout_timeout => 0.5}))
@connection_count = 0
@timed_out = 0
threads.times do
View
2  activerecord/test/cases/reaper_test.rb
@@ -64,7 +64,7 @@ def test_connection_pool_starts_reaper
spec.config[:reaping_frequency] = 0.0001
pool = ConnectionPool.new spec
- pool.timeout = 0
+ pool.dead_connection_timeout = 0
conn = pool.checkout
count = pool.connections.length
Something went wrong with that request. Please try again.