Mysql2 Adapter and wait_timeout config #2894

Closed
fonzo14 opened this Issue Sep 6, 2011 · 9 comments

Comments

Projects
None yet
6 participants

fonzo14 commented Sep 6, 2011

To me, the mysql2 adapter is using the wait_timeout config in a wrong way. It is used to set the mysql @@wait_timeout parameter :

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)

This mysql variable sets the time mysql keeps alive an unactive connection before closing it .

The point is that the wait_timeout config is also used in connection_pool as the time a client will wait for a pool connection :
@timeout = spec.config[:wait_timeout] || 5

So we have the same config parameter for both use case. And if we normally want a long mysql @@wait_timeout variable (the mysql default value is 8 hours), we want a much smaller timeout for the client.

I'm not sure using the same value for both settings is a good choice

Contributor

isaacsanders commented Apr 28, 2012

@fonzo14 Is this still an issue?

fonzo14 commented Apr 29, 2012

Hi @isaacsanders

Well, I'm currently not using the mysql2 adapter. But After a quick look at the AR code, the 'problem' seems to be still there.

You just have to look at the comments :

in connection_pool

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

in mysql2_adapter :

increase timeout so mysql server doesn't disconnect us
wait_timeout = @config[:wait_timeout]

Contributor

isaacsanders commented Apr 29, 2012

Do you have time to contribute a patch for this?

Contributor

jrochkind commented May 23, 2012

Oops, just realized I filed a duplicate at #6441 . More information there. Sorry, missed this issue before filing the new ticket.

It's definitely still a problem.

I can submit a patch, but I'm not sure if I need to submit a patch to 3-2-stable, or master, or both, and exactly what the nature of the patch should be. I want to avoid doing work that is going to conflict with maintainers overall plans or not be useful. Advice welcome.

In particular, clearly the config key for either mysql2 or connectionpool needs to be changed -- which will be backwards-incompat. I'm not sure which one should be changed. (The current behavior is going to lead to undesirable outcome 99% of the time for people using mysql2, so there's no practical backwards-compat problem in breaking 'current behavior' for them -- but could be for people not using mysql2 but using the wait_timeout key in connectionpool. This implies that it would be better to change the key for mysql2, rather than for connectionpool -- the problem though, is that the key name wait_timeout makes more sense for mysql2 than it does for connectionpool, if I were starting from scratch I'd change the name in connectionpool).

Contributor

dmathieu commented May 23, 2012

Submit your patch to master. Once it's accepted, you'll see to backport it.
Could you close this issue as a duplicate please ?

Contributor

jrochkind commented May 23, 2012

I should close this one, or the new one?

Any way to get advice from maintainers on which key should be changed? I'm leaning toward doing one thing in master, and another in 3-2-stable. But I have no idea if another 3-2 release is ever going to happen, I don't want to waste time thinking/implenting for 3-2-stable if it's not gonna matter.

Contributor

dmathieu commented May 23, 2012

The other one seems more detailed than this one. I guess it'd be better to close this one.

There probably won't be any new release of 3.2 (there will be only for security fixes).
The two codebases aren't that different though. It shouldn't take hours to backport from one to the other.
Moreover, if someone is running on 3.2 and has your problem, if you backport, he can use rails from git even if nothing has been released.

Contributor

jrochkind commented May 23, 2012

Okay, thanks. I'll try to submit a pull to master soon.

I actually don't have privs to close this one, I can close the one I
filed, the newer one, even though it's more detailed?

On May 23, 2012, at 9:35 AM, Damien Mathieu wrote:

The other one seems more detailed than this one. I guess it'd be
better to close this one.

There probably won't be any new release of 3.2 (there will be only
for security fixes).
The two codebases aren't that different though. It shouldn't take
hours to backport from one to the other.
Moreover, if someone is running on 3.2 and has your problem, if you
backport, he can use rails from git even if nothing has been released.


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

Closing this in favor of #6441, thanks!

jrochkind added a commit to jrochkind/rails that referenced this issue May 23, 2012

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment