Use Thread.handle_interrupt to protect query

[tamird]

When available, we prevent Timeout.timeout from corrupting
connections using Thread.handle_interrupt. Fixes #542.

/cc @sodabrew @eam @rudle @ggilder @zanker

[sodabrew]

This is amazing, thank you. Does this change mean that d0a5199 can be reverted, or is that still a separate issue?

[tamird]

yeah, probably. let me push a commit

[tamird]

@sodabrew accidentally broke 1.8.7 with a stabby lambda -- should be green now

[tamird]

@sodabrew i think this is the real behaviour we want to demonstrate and test for - we will protect the connection from regular Timeout.timeout but not weird arbitrary exceptions

[tamird]

@sodabrew thoughts?

[tamird]

@sodabrew @brianmario how can I help get this merged?

[sodabrew]

I've been super busy at work this week and have a kid with an ear infection at home. This is a high priority item on a queue that's not getting any cycles :)

[brianmario]

Apologies from me as well, I need to read up on the handle_interrupt
stuff to think about what the implications are. Could you provide a little
more information about what the side effects of this are?

On Fri, Mar 13, 2015 at 9:41 AM, Aaron Stone notifications@github.com
wrote:

I've been super busy at work this week and have a kid with an ear
infection at home. This is a high priority item on a queue that's not
getting any cycles :)

Reply to this email directly or view it on GitHub
#592 (comment).

[tamird]

@brianmario is this helpful: http://ruby-doc.org/core-2.2.1/Thread.html#method-c-handle_interrupt-label-Guarding+from+Timeout-3A-3AError ?

[tamird]

@sodabrew @brianmario ping, any thoughts on this?

[brianmario]

Sorry I've been meaning to get to this but haven't had the time to spend on
thinking about this. Since this is a newer API I just want to make sure I
fully understand the implications. We've had a lot of issues around this
codepath and timeouts/signals in the past and I'd rather not deal with
introducing any new ones if we can ;)

On Wed, Mar 18, 2015 at 8:39 AM, Tamir Duberstein notifications@github.com
wrote:

@sodabrew https://github.com/sodabrew @brianmario
https://github.com/brianmario ping, any thoughts on this?

Reply to this email directly or view it on GitHub
#592 (comment).

[sodabrew]

If I understand this correctly, the query simply won't be interrupted by a timeout? The wall-clock time for this would be 5 seconds:

expect { Timeout.timeout(1) { @client.query('SELECT SLEEP(5)') } }.to raise_error(Timeout::Error)

My major concern is documenting that Mysql2::Client#query cannot be interrupted by a Timeout.timeout. This may surprise some users and they will ask for an alternative way to prevent their application from blocking forever on a database query. In many cases, returning failure in a timely manner (even causing the screwy connection state) is more important than succeeding.

I've read this over a few times, and I like it. I think readability is improved by handling the query_options merge in Ruby space before kicking over to C, nice work on that.

[sodabrew]

Turns out upstream is working on this! MySQL 5.7, Percona 5.6, and MariaDB 10.1 all have max_statement_time for SELECT slated for future GA releases (the syntax looks like it will change a bit before finalized):
http://mysqlserverteam.com/server-side-select-statement-timeouts/
https://mariadb.com/kb/en/mariadb/aborting-statements/

I figure this means a hard timeout at the client end was never supposed to work in the first place.

[tamird]

@sodabrew nice find. Is this good to land?

[brianmario]

My major concern is documenting that Mysql2::Client#query cannot be interrupted by a Timeout.timeout.

This was my concern as well. If a run-away query is holding up a ruby process in a pre-fork environment, a user could continue making requests until all of the processes were starved and take the site down.

Where otherwise, if we let the caller timeout a query with a Timeout block we can document that they must close and create a new connection as the protocol would be in a broken state. That's a little more complex for them to deal with but I'd say it's a fair trade vs not being able to stop their processes from being used up by a query they can't stop.

What do you guys think?

@charliesome @dbussink @samlambert @tmm1 do you guys have any thoughts on this?

That new max_statement_time definitely looks interesting and feels like a much better way to handle this. That way the server can reset its protocol state and allow more commands to be sent on that connection.

[haileys]

The big issue I see with using max_statement_time instead is that it only works if the MySQL server is up and responsive. It won't help protect us against timing out due to other network errors.

[tamird]

Doesn't read_timeout already provide a sane variety of timeout? https://github.com/brianmario/mysql2/blob/master/spec/mysql2/client_spec.rb#L416:L421

[JoshMcKin]

read_timeout only interrupts the connection it does not terminate the query. We use it extensively for some sketch setups during scaling projects and often see the timeout error in the rails project but the query is persists in Mysql. I image max_statement_time is will actually terminate the query.

[tamird]

@JoshMcKin agreed, but Timeout.timeout also does not terminate the query - I'm only suggesting that it's OK to make #query uninterruptible because read_timeout provides a different mechanism for preventing pre-fork worker starvation, addressing @brianmario's concern.

[JoshMcKin]

Hi @tamird , sorry if I wasn't clear my intention was to agree read_timeout works well ensure long connections don't block a Rails process indefinitely.

[tamird]

ping @brianmario, @charliesome et al. any more thoughts?

[sodabrew]

Would you do a rebase to master now that prepared statements have landed?

[tamird]

Rebased.

[sodabrew]

Locally, I am getting this unit test failure. Ruby 2.0 from OS X Yosemite, MySQL 5.5.

  1) Mysql2::Client#query when a non-standard exception class is raised should handle Timeouts without leaving the connection hanging if reconnect is true
     Failure/Error: expect { client.query('SELECT 1') }.to_not raise_error
       expected no Exception, got #<Mysql2::Error: Lost connection to MySQL server during query>
     # ./spec/mysql2/client_spec.rb:485:in `block (4 levels) in <top (required)>'

This is the test where :reconnect => true is passed to the connection. Odd. Debugging.

[tamird]

Any luck, @sodabrew?

[sodabrew]

I found that my local MySQL always fails to reconnect – will have to track down whatever else I was hacking on. Not a blocker to merge. Merging now!

[tamird]

\o/ this is huge