Skip to content

Commit

Permalink
Translate Trilogy syscall errors as conn failed
Browse files Browse the repository at this point in the history
At GitHub we get a fair number of Trilogy `ETIMEDOUT` errors (for known
reasons that we might be able to improve somewhat, but I doubt we'll
make them go away entirely). These are very much retryable network
errors, so it'd be handy if these `ETIMEDOUT` errors were translated to
`ConnectionFailed` instead of `StatementInvalid`, making them
`retryable_connection_error`s.

https://github.com/rails/rails/blob/ed2bc92b82ddc111150cdf48bb646fd97b3baacb/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb#L1077

We're already translating `ECONNRESET` (via matching on the error
message) and `EPIPE` to `ConnectionFailed`. Rather than adding another
case, this commit treats all of the Trilogy `SystemCallError` subclasses
as `ConnectionFailed`.

This commit also drops `TRILOGY_INVALID_SEQUENCE_ID`,
`TRILOGY_UNEXPECTED_PACKET` as `ConnectionFailed`. These are not
transient network errors we might want to retry. These are MySQL
client/server protocol errors, likely indicating something we need to
fix in Trilogy.

We should be able to simplify this exception translation further once we
bump the required Trilogy version to 2.6, but I figured I'd start out
with this minimal change that works on Trilogy 2.4 as well.
  • Loading branch information
composerinteralia committed Nov 28, 2023
1 parent ed2bc92 commit a6c3c50
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -205,10 +205,10 @@ def translate_exception(exception, message:, sql:, binds:)
end

case exception
when Errno::EPIPE, SocketError, IOError
when SocketError, IOError
return ConnectionFailed.new(message, connection_pool: @pool)
when ::Trilogy::Error
if /Connection reset by peer|TRILOGY_CLOSED_CONNECTION|TRILOGY_INVALID_SEQUENCE_ID|TRILOGY_UNEXPECTED_PACKET/.match?(exception.message)
if exception.message.include?("TRILOGY_CLOSED_CONNECTION") || exception.is_a?(SystemCallError)
return ConnectionFailed.new(message, connection_pool: @pool)
end
end
Expand Down
32 changes: 32 additions & 0 deletions activerecord/test/cases/adapters/trilogy/trilogy_adapter_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,38 @@ class TrilogyAdapterTest < ActiveRecord::TrilogyTestCase
ActiveRecord::Base.establish_connection :arunit
end

test "EPIPE raises ActiveRecord::ConnectionFailed" do
assert_raises(ActiveRecord::ConnectionFailed) do
@conn.raw_connection.stub(:query, -> (*) { raise Trilogy::SyscallError::EPIPE }) do
@conn.execute("SELECT 1")
end
end
end

test "ETIMEDOUT raises ActiveRecord::ConnectionFailed" do
assert_raises(ActiveRecord::ConnectionFailed) do
@conn.raw_connection.stub(:query, -> (*) { raise Trilogy::SyscallError::ETIMEDOUT }) do
@conn.execute("SELECT 1")
end
end
end

test "ConnectionRefusedError raises ActiveRecord::ConnectionFailed" do
assert_raises(ActiveRecord::ConnectionFailed) do
@conn.raw_connection.stub(:query, -> (*) { raise Trilogy::ConnectionRefusedError }) do
@conn.execute("SELECT 1")
end
end
end

test "ConnectionResetError raises ActiveRecord::ConnectionFailed" do
assert_raises(ActiveRecord::ConnectionFailed) do
@conn.raw_connection.stub(:query, -> (*) { raise Trilogy::ConnectionResetError }) do
@conn.execute("SELECT 1")
end
end
end

# Create a temporary subscription to verify notification is sent.
# Optionally verify the notification payload includes expected types.
def assert_notification(notification, expected_payload = {}, &block)
Expand Down

0 comments on commit a6c3c50

Please sign in to comment.