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`.

Unfortunately `Trilogy::TimeoutError` is a subclass of `ETIMEDOUT`, so
we have to move the `AdapterTimeout` check into the case statement so we
can break out to the `super` call in the case of `Trilogy::TimeoutError`
that does have an error code (1205, i.e. lock timeout). Cleaning up the
various Trilogy timeout errors is a project for another day.

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 7f01c8c
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 5 deletions.
Expand Up @@ -194,9 +194,6 @@ def get_full_version
end

def translate_exception(exception, message:, sql:, binds:)
if exception.is_a?(::Trilogy::TimeoutError) && !exception.error_code
return ActiveRecord::AdapterTimeout.new(message, sql: sql, binds: binds, connection_pool: @pool)
end
error_code = exception.error_code if exception.respond_to?(:error_code)

case error_code
Expand All @@ -205,10 +202,15 @@ def translate_exception(exception, message:, sql:, binds:)
end

case exception
when Errno::EPIPE, SocketError, IOError
when ::Trilogy::TimeoutError
if !error_code
return ActiveRecord::AdapterTimeout.new(message, sql: sql, binds: binds, connection_pool: @pool)
end
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 /TRILOGY_CLOSED_CONNECTION|TRILOGY_INVALID_SEQUENCE_ID|TRILOGY_UNEXPECTED_PACKET/.match?(exception.message) ||
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
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 7f01c8c

Please sign in to comment.