Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't swallow exceptions in transctional statements #12779

Conversation

dougbarth
Copy link
Contributor

The MySQL connection adapater swallows all StandardError exceptions, which includes Mysql::Error and Mysql2::Error. The comment in the exception clause claims errors thrown here indicate that transactions aren't supported by the server but that isn't necessarily true. It's possible the MySQL server has gone away and swallowing a failed commit may let the application return a successful response when the data has not been saved. Also, replication libraries like Galera require that the application handle exceptions thrown at BEGIN/COMMIT.

I'm unable to determine what version of MySQL threw an exception for transactional statements. I tried as far back as 3.23.49 with InnoDB disabled but BEGIN & COMMIT statements do not throw an error. If there's a real case for this logic to continue, we could instead push this behavior into a configuration setting.

The exception swallowing has been there since the beginning: db045db

Backstory

I found this behavior while moving our application to Percona XtraDB Cluster, which uses Galera for synchronous replication. We found a case where a transaction failed the certification process, and therefore threw an exception when COMMIT was called, but the exception was swallowed and the response to the client was an erroneous 200.

The MySQL connection adapater swallows all StandardError exceptions,
which includes Mysql::Error and Mysql2::Error. The comment in the
exception clause claims errors thrown here indicate that transactions
aren't supported by the server but that isn't necessarily true. It's
possible the MySQL server has gone away and swallowing a failed commit
may let the application return a successful response when the data has
not been saved. Also, replication libraries like Galera require that the
application handle exceptions thrown at BEGIN/COMMIT.

I'm unable to determine what version of MySQL threw an exception for
transactional statements. I tried as far back as 3.23.49 with InnoDB
disabled but BEGIN & COMMIT statements do not throw an error. If there's
a real case for this logic to continue, we could instead push this
behavior into a configuration setting.

The exception swallowing has been there since the beginning:
db045db
@tenderlove
Copy link
Member

I'm positive on this patch, but I dislike adding stubs on the connection. What if we change the implementation to call something other than execute or exec_query? The stubs are tightly coupled to the implementation.

IMO these tests should be removed, and we should just remove the rescue statements. I don't see why we're rescuing all exceptions in those places.

@rafaelfranca
Copy link
Member

👍

From PR, @tenderlove would prefer to not maintain these tests.
@dougbarth
Copy link
Contributor Author

@tenderlove Tests yanked.

tenderlove added a commit that referenced this pull request Nov 15, 2013
…ng_transactional_statements_in_mysql

Don't swallow exceptions in transctional statements
@tenderlove tenderlove merged commit 314e2cc into rails:master Nov 15, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants