Prevent marking broken connections as verified
Related to https://github.com/rails/rails/pull/49802 Prior to this commit `with_raw_connection` would always mark the connection as `@verified = true` after yielding it. But yielding a connection is not enough to determine that a connection is healthy. Marking a broken connection as verified can prevent further reconnects, leaving us with a broken connection for longer than expected. For example, * Connection is broken and not verified * Quote string succeeds, and marks the broken connection as verified * Because the connection is verified, querying will not trigger a reconnect * Query fails and the connection is no longer verified * Repeat ad infinitum (or until we reach a query with `allow_retry: true`, or one that isn't preceded by a call to quote string) This commit solves the problem by only marking the connection as verified if we have definitively exercised that connection. This is safer, but it does put more responsibility on each individual adapter. If a call to `verified!` is missing things should still work OK, but we'll end up pinging the connection more to see if it is active.
This commit is contained in:
Daniel Colson
parent
9c8585156e
commit
a180823195
@ -977,7 +977,11 @@ def reconnect_can_restore_state?
|
||||
# If +allow_retry+ is true, a connection-related exception will
|
||||
# cause an automatic reconnect and re-run of the block, up to
|
||||
# the connection's configured +connection_retries+ setting
|
||||
# and the configured +retry_deadline+ limit.
|
||||
# and the configured +retry_deadline+ limit. (Note that when
|
||||
# +allow_retry+ is true, it's possible to return without having marked
|
||||
# the connection as verified. If the block is guaranteed to exercise the
|
||||
# connection, consider calling `verified!` to avoid needless
|
||||
# verification queries in subsequent calls.)
|
||||
#
|
||||
# If +materialize_transactions+ is false, the block will be run without
|
||||
# ensuring virtual transactions have been materialized in the DB
|
||||
@ -1028,9 +1032,7 @@ def with_raw_connection(allow_retry: false, materialize_transactions: true)
|
||||
end
|
||||
|
||||
begin
|
||||
result = yield @raw_connection
|
||||
@verified = true
|
||||
result
|
||||
yield @raw_connection
|
||||
rescue => original_exception
|
||||
translated_exception = translate_exception_class(original_exception, nil, nil)
|
||||
invalidate_transaction(translated_exception)
|
||||
@ -1065,6 +1067,13 @@ def with_raw_connection(allow_retry: false, materialize_transactions: true)
|
||||
end
|
||||
end
|
||||
|
||||
# Mark the connection as verified. Call this inside a
|
||||
# `with_raw_connection` block only when the block is guaranteed to
|
||||
# exercise the raw connection.
|
||||
def verified!
|
||||
@verified = true
|
||||
end
|
||||
|
||||
def retryable_connection_error?(exception)
|
||||
exception.is_a?(ConnectionNotEstablished) || exception.is_a?(ConnectionFailed)
|
||||
end
|
||||
|
||||
@ -102,6 +102,7 @@ def raw_execute(sql, name, async: false, allow_retry: false, materialize_transac
|
||||
with_raw_connection(allow_retry: allow_retry, materialize_transactions: materialize_transactions) do |conn|
|
||||
sync_timezone_changes(conn)
|
||||
result = conn.query(sql)
|
||||
verified!
|
||||
handle_warnings(sql)
|
||||
result
|
||||
end
|
||||
@ -130,6 +131,8 @@ def exec_stmt_and_free(sql, name, binds, cache_stmt: false, async: false)
|
||||
result = ActiveSupport::Dependencies.interlock.permit_concurrent_loads do
|
||||
stmt.execute(*type_casted_binds)
|
||||
end
|
||||
verified!
|
||||
result
|
||||
rescue ::Mysql2::Error => e
|
||||
if cache_stmt
|
||||
@statements.delete(sql)
|
||||
|
||||
@ -16,7 +16,9 @@ def query(sql, name = nil) # :nodoc:
|
||||
|
||||
log(sql, name) do
|
||||
with_raw_connection do |conn|
|
||||
conn.async_exec(sql).map_types!(@type_map_for_results).values
|
||||
result = conn.async_exec(sql).map_types!(@type_map_for_results).values
|
||||
verified!
|
||||
result
|
||||
end
|
||||
end
|
||||
end
|
||||
@ -51,6 +53,7 @@ def raw_execute(sql, name, async: false, allow_retry: false, materialize_transac
|
||||
log(sql, name, async: async) do
|
||||
with_raw_connection(allow_retry: allow_retry, materialize_transactions: materialize_transactions) do |conn|
|
||||
result = conn.async_exec(sql)
|
||||
verified!
|
||||
handle_warnings(result)
|
||||
result
|
||||
end
|
||||
|
||||
@ -893,7 +893,9 @@ def exec_no_cache(sql, name, binds, async:, allow_retry:, materialize_transactio
|
||||
type_casted_binds = type_casted_binds(binds)
|
||||
log(sql, name, binds, type_casted_binds, async: async) do
|
||||
with_raw_connection do |conn|
|
||||
conn.exec_params(sql, type_casted_binds)
|
||||
result = conn.exec_params(sql, type_casted_binds)
|
||||
verified!
|
||||
result
|
||||
end
|
||||
end
|
||||
end
|
||||
@ -908,7 +910,9 @@ def exec_cache(sql, name, binds, async:, allow_retry:, materialize_transactions:
|
||||
type_casted_binds = type_casted_binds(binds)
|
||||
|
||||
log(sql, name, binds, type_casted_binds, stmt_key, async: async) do
|
||||
conn.exec_prepared(stmt_key, type_casted_binds)
|
||||
result = conn.exec_prepared(stmt_key, type_casted_binds)
|
||||
verified!
|
||||
result
|
||||
end
|
||||
end
|
||||
rescue ActiveRecord::StatementInvalid => e
|
||||
|
||||
@ -50,6 +50,7 @@ def internal_exec_query(sql, name = nil, binds = [], prepare: false, async: fals
|
||||
stmt.bind_params(type_casted_binds)
|
||||
records = stmt.to_a
|
||||
end
|
||||
verified!
|
||||
|
||||
build_result(columns: cols, rows: records)
|
||||
end
|
||||
@ -76,7 +77,9 @@ def begin_isolated_db_transaction(isolation) # :nodoc:
|
||||
def begin_db_transaction # :nodoc:
|
||||
log("begin transaction", "TRANSACTION") do
|
||||
with_raw_connection(allow_retry: true, materialize_transactions: false) do |conn|
|
||||
conn.transaction
|
||||
result = conn.transaction
|
||||
verified!
|
||||
result
|
||||
end
|
||||
end
|
||||
end
|
||||
@ -112,7 +115,9 @@ def high_precision_current_timestamp
|
||||
def raw_execute(sql, name, async: false, allow_retry: false, materialize_transactions: false)
|
||||
log(sql, name, async: async) do
|
||||
with_raw_connection(allow_retry: allow_retry, materialize_transactions: materialize_transactions) do |conn|
|
||||
conn.execute(sql)
|
||||
result = conn.execute(sql)
|
||||
verified!
|
||||
result
|
||||
end
|
||||
end
|
||||
end
|
||||
@ -133,7 +138,9 @@ def execute_batch(statements, name = nil)
|
||||
|
||||
log(sql, name) do
|
||||
with_raw_connection do |conn|
|
||||
conn.execute_batch2(sql)
|
||||
result = conn.execute_batch2(sql)
|
||||
verified!
|
||||
result
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
@ -47,6 +47,7 @@ def raw_execute(sql, name, async: false, allow_retry: false, materialize_transac
|
||||
with_raw_connection(allow_retry: allow_retry, materialize_transactions: materialize_transactions) do |conn|
|
||||
sync_timezone_changes(conn)
|
||||
result = conn.query(sql)
|
||||
verified!
|
||||
handle_warnings(sql)
|
||||
result
|
||||
end
|
||||
|
||||
@ -612,6 +612,26 @@ def teardown
|
||||
assert_predicate @connection, :active?
|
||||
end
|
||||
|
||||
test "quoting a string on a 'clean' failed connection will not prevent reconnecting" do
|
||||
remote_disconnect @connection
|
||||
|
||||
@connection.clean! # this simulates a fresh checkout from the pool
|
||||
|
||||
# Clean did not verify / fix the connection
|
||||
assert_not_predicate @connection, :active?
|
||||
|
||||
# Quote string will not verify a broken connection (although it may
|
||||
# reconnect in some cases)
|
||||
Post.connection.quote_string("")
|
||||
|
||||
# Because the connection hasn't been verified since checkout,
|
||||
# and the query cannot safely be retried, the connection will be
|
||||
# verified before querying.
|
||||
Post.delete_all
|
||||
|
||||
assert_predicate @connection, :active?
|
||||
end
|
||||
|
||||
test "querying after a failed query restores and succeeds" do
|
||||
Post.first # Connection verified (and prepared statement pool populated if enabled)
|
||||
|
||||
|
||||
Loading…
Reference in New Issue
Block a user