Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

ActiveRecord `wait_timeout` used inconsistently by mysql2 and ConnectionPool #6441

Closed
jrochkind opened this Issue · 7 comments

3 participants

@jrochkind

Rails 3.2.3

The mysql2 adapter and the ConnectionPool both use a value wait_timeout from the connection spec (eg in database.yml).

However, they use this value for very different things, with very different defaults.

ConnectionPool uses wait_timeout to decide how long to wait for a connection checkout on a full connection pool, and it defaults to 5 seconds.

+wait_timeout+: number of seconds to block and wait for a connection before giving up and raising a timeout error (default 5 seconds).

mysql2_adapter uses this same wait_timeout , but passes it directly to mysql, and defaults to a much larger 2592000!!

    # increase timeout so mysql server doesn't disconnect us
    wait_timeout = @config[:wait_timeout]
    wait_timeout = 2592000 unless wait_timeout.is_a?(Fixnum)
    variable_assignments << "@@wait_timeout = #{wait_timeout}"

    execute("SET #{variable_assignments.join(', ')}", :skip_logging)

wait_timeout here is sent directly to MySQL for it's own configuration, where it means

The number of seconds the server waits for activity on a noninteractive connection before closing it.

You seldom are going to want ConnectionPool's wait_timeout to be the same as MySQL's -- and by default, they are very different. But if you don't want the defaults, you can't really change either one without changing the other to be the same, which you will not want.

My problem

ConnectionPool's default of 5 seconds is too long for me, I want to change it to 2 seconds. In order to do this, I need to add wait_timeout: 2 to my connection spec in database.yml. But in doing this, I've also changed mysql's wait_timeout from 2592000 (!!) to 2, which results in mysql2 closing my connections all the time, which makes ActiveRecord raise.

This is a problem, that these two very different timeout values, which you seldom are going to want to be the same value (as evidenced by their very different defaults), are both controleld by the same connection spec config.

One or the other of the connection spec keys needs to be changed.

What to do?

Advice welcome. I could submit a patch against 3-2-stable, but I'm not sure if there's planned to be another 3.2.x release?

Master/4 has changed enough to make it unclear if this problem still exists -- or if it's possibly changed to a different problems with colliding names used for different things -- I think master ConnectionPool just uses 'timeout', somewhat harder to grep to see if any of the various db adapters use that key for something entirely different or not) . Would require some investigation, unless perhaps there's a committer who understands what's going on who can chime in.

@jrochkind

There's a really hacky and fragile workaround one can do with current rails code to change ConnectionPool's timeout without changing mysql2's.

ConnectionPool is willing to call #to_i on the wait_timeout value.

However, mysql2 will only use a value from a connection spec (rather than it's default) if it's a Fixnum.

So if you set wait_timeout: "2" using a yaml string, then ConnectionPool ought to use it's #to_i value instead of it's default, but mysql2 ought to ignore it and use mysql2's default.

I haven't tested this yet. But even if it works, this is really hacky and opaque and fragile, sort of an accident of implementation one should not count on.

@jrochkind jrochkind referenced this issue from a commit in jrochkind/rails
@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
@jrochkind jrochkind closed this
@tenderlove
Owner

I could submit a patch against 3-2-stable, but I'm not sure if there's planned to be another 3.2.x release?

Yes, there will be another release of 3.2. It wouldn't make sense to drop support for the latest released version. That would mean that we don't support any version of rails. ;-)

@tenderlove tenderlove reopened this
@tenderlove
Owner

I think we should add a new key. Something like idle_connection_timeout. We should have this key default to the value of wait_timeout in order to remain backwards compatible. I think it's OK if this key is specific to mysql2.

@jrochkind

The thing about the current mysql2 key (which is mysql2-specific) wait_timeout though, is that it matches the name of the key in the MySQL rdbms itself, which kind of makes sense -- short of providing a new standard idle_timeout key that is standard accross all AR adapters -- Which wouldn't be backwards compat anyway. If every adapter is going to use their own custom keys (which they do currently), it makes sense for those custom keys to match the language of the rdbms.

Here's one way I thought of to do it completely backwards compat, for rails 3-2-stable. In ConnectionPool,

 @timeout = spec.config[:checkout_timeout] || spec.config[:wait_timeout] || 5

It's completely backwards compat if you didn't have a :checkout_timeout key already (and why would you have? It didn't mean anything to any part of AR).

But if you're bit by the problem in this ticket, you can define a :checkout_timeout key to take precedence (which will be the same key rails4 ConnectionPool will use).

There could be a deprecation warning only iff spec.config.has_key?(:wait_timeout) && ! spec.config.has_key?(:checkout_timeout), saying something somewhat confusing like:

:wait_timeout as a directive for ConnectionPool is deprecated. If you meant wait_timeout for mysql2, please add a :checkout_timeout key checkout_timeout: 5 to keep it from applying to ConnectionPool. If you meant wait_timeout for ConnectionPool, please change it to checkout_timeout.

@jrochkind jrochkind referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@jrochkind jrochkind referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@jrochkind jrochkind referenced this issue from a commit in jrochkind/rails
@jrochkind jrochkind ConnectionPool accepts spec key 'checkout_timeout'
Backport of #6441 cb6f839 . Old 'wait_timeout' is still supported,
but conflicts with mysql2 using that spec key for different thing.
'checkout_timeout' can now be used taking precedence for ConnectionPool
over 'wait_timeout'.
3908706
@rafaelfranca

Closed by #7684

@jbraeuer jbraeuer referenced this issue in aws/opsworks-cookbooks
Closed

Include pool size in database.yml.erb #99

@jrochkind

@rafaelfranca, hi, sorry this is over a year old but i"m just getting to upgrading my actual apps to rails4.

You marked this issue "closed by #7684", but "#7684" was meant to be a backport of this PR (which was against master, which at the time was leading up to rails 4.0) to the rails-3-2 branch.

I'm left confused as to whether this functionality made it into master/rails4... or if we accidentally fixed it in rails-3-2 but left it unfixed in rails4?

@jrochkind

Ah wait, nevermind, there's lots of issues/PR's in here, can get confused, but I think this was really closed by PR #6463 maybe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.