Permalink
Browse files

Don't swallow exceptions in transctional statements

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
  • Loading branch information...
1 parent 44406d1 commit 5d870c929157b7c7949c9356d4f88b97c8848ec3 @dougbarth dougbarth committed Nov 5, 2013
View
8 activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
@@ -314,27 +314,19 @@ def update_sql(sql, name = nil) #:nodoc:
def begin_db_transaction
execute "BEGIN"
- rescue
- # Transactions aren't supported
end
def begin_isolated_db_transaction(isolation)
execute "SET TRANSACTION ISOLATION LEVEL #{transaction_isolation_levels.fetch(isolation)}"
begin_db_transaction
- rescue
- # Transactions aren't supported
end
def commit_db_transaction #:nodoc:
execute "COMMIT"
- rescue
- # Transactions aren't supported
end
def rollback_db_transaction #:nodoc:
execute "ROLLBACK"
- rescue
- # Transactions aren't supported
end
# In the simple case, MySQL allows us to place JOINs directly into the UPDATE
View
2 activerecord/lib/active_record/connection_adapters/mysql_adapter.rb
@@ -468,8 +468,6 @@ def exec_delete(sql, name, binds)
def begin_db_transaction #:nodoc:
exec_query "BEGIN"
- rescue Mysql::Error
- # Transactions aren't supported
end
private
View
26 activerecord/test/cases/adapters/mysql/connection_test.rb
@@ -40,6 +40,11 @@ def test_no_automatic_reconnection_after_timeout
@connection.update('set @@wait_timeout=1')
sleep 2
assert !@connection.active?
+
+ # Repair all fixture connections so other tests won't break.
+ @fixture_connections.each do |c|
+ c.verify!
+ end
end
def test_successful_reconnection_after_timeout_with_manual_reconnect
@@ -159,6 +164,27 @@ def test_mysql_set_session_variable_to_default
end
end
+ def test_mysql_begin_db_transaction_can_throw_an_exception
+ @connection.expects(:exec_query).with('BEGIN').raises('OH NOES')
+ assert_raise RuntimeError do
+ @connection.begin_db_transaction
+ end
+ end
+
+ def test_mysql_commit_db_transaction_can_throw_an_exception
+ @connection.expects(:execute).with('COMMIT').raises('OH NOES')
+ assert_raise RuntimeError do
+ @connection.commit_db_transaction
+ end
+ end
+
+ def test_mysql_rollback_db_transaction_can_throw_an_exception
+ @connection.expects(:execute).with('ROLLBACK').raises('OH NOES')
+ assert_raise RuntimeError do
+ @connection.rollback_db_transaction
+ end
+ end
+
private
def run_without_connection
View
26 activerecord/test/cases/adapters/mysql2/connection_test.rb
@@ -18,6 +18,11 @@ def test_no_automatic_reconnection_after_timeout
@connection.update('set @@wait_timeout=1')
sleep 2
assert !@connection.active?
+
+ # Repair all fixture connections so other tests won't break.
+ @fixture_connections.each do |c|
+ c.verify!
+ end
end
def test_successful_reconnection_after_timeout_with_manual_reconnect
@@ -84,6 +89,27 @@ def test_logs_name_rename_column_sql
@connection.execute "DROP TABLE `bar_baz`"
end
+ def test_mysql_begin_db_transaction_can_throw_an_exception
+ @connection.expects(:execute).with('BEGIN').raises('OH NOES')
+ assert_raise RuntimeError do
+ @connection.begin_db_transaction
+ end
+ end
+
+ def test_mysql_commit_db_transaction_can_throw_an_exception
+ @connection.expects(:execute).with('COMMIT').raises('OH NOES')
+ assert_raise RuntimeError do
+ @connection.commit_db_transaction
+ end
+ end
+
+ def test_mysql_rollback_db_transaction_can_throw_an_exception
+ @connection.expects(:execute).with('ROLLBACK').raises('OH NOES')
+ assert_raise RuntimeError do
+ @connection.rollback_db_transaction
+ end
+ end
+
private
def run_without_connection

0 comments on commit 5d870c9

Please sign in to comment.