Introduce EOFError and make SyscallError < ConnectionError #118
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a new Ruby error to represent the TRILOGY_CLOSED_CONNECTION status code, which is the status returned when read returns 0, write hits an EPIPE, or when attempting to communicate through a socket after it was shutdown/closed due to a previous error. This subclasses BaseConnectionError so that it can be caught by `rescue Trilogy::ConnectionError` (which is what we'd like to recommend for detecting network/communication issues) and will always include the TRILOGY_CLOSED_CONNECTION as part of the message for backwards compatibility, as that's how currently Rails and others have been detecting it. Previously this would have been raised as a QueryError.
composerinteralia
approved these changes
Aug 23, 2023
| @@ -112,7 +112,13 @@ class SSLError < BaseError | |||
| include ConnectionError | |||
| end | |||
|
|
|||
| # Raised on attempt to use connection which was explicitly closed by the user | |||
There was a problem hiding this comment.
Thank you! I've been confused by this in the past.
composerinteralia
added a commit
to composerinteralia/rails
that referenced
this pull request
Jan 26, 2024
This commit makes a few changes to our trilogy error translation: * trilogy-libraries/trilogy#118 introduced `Trilogy::EOFError` which we can use instead of matching on `TRILOGY_CLOSED_CONNECTION`. * trilogy-libraries/trilogy#15 introduced `Trilogy::ConnectionClosed`, which inherits from `IOError` for backwards compatibility. As far as I can tell that's the only `IOError` trilogy can raise, so this commit rescues the trilogy-specific error instead. * As far as I can tell Trilogy does not raise `SocketError`, so don't bother translating that * Don't treat TRILOGY_UNEXPECTED_PACKET as a connection error. If we get this, it's probably a bug in trilogy that we should fix. I'd like to eventually get rid of TRILOGY_INVALID_SEQUENCE_ID too, but we're currently relying on it in a few tests (related to trilogy missing caching_sha2_password auth support, if I recall correctly) I'm kinda hoping we'll eventually be able to simplify this to something like: ```rb if exception.is_a?(Trilogy::ConnectionError) ConnectionFailed.new(message, connection_pool: @pool) else super end ``` but we'd need more changes to trilogy before that is possible.
composerinteralia
added a commit
to composerinteralia/rails
that referenced
this pull request
Jan 26, 2024
This commit makes a few changes to our trilogy error translation: * trilogy-libraries/trilogy#118 introduced `Trilogy::EOFError` which we can use instead of matching on `TRILOGY_CLOSED_CONNECTION`. * trilogy-libraries/trilogy#15 introduced `Trilogy::ConnectionClosed`, which inherits from `IOError` for backwards compatibility. As far as I can tell that's the only `IOError` trilogy can raise, so this commit rescues the trilogy-specific error instead. * As far as I can tell Trilogy does not raise `SocketError`, so don't bother translating that * Don't treat TRILOGY_UNEXPECTED_PACKET as a connection error. If we get this, it's probably a bug in trilogy that we should fix. I'd like to eventually get rid of TRILOGY_INVALID_SEQUENCE_ID too, but we're currently relying on it in a few tests (related to trilogy missing caching_sha2_password auth support, if I recall correctly) I'm kinda hoping we'll eventually be able to simplify this to something like: ```rb if exception.is_a?(Trilogy::ConnectionError) ConnectionFailed.new(message, connection_pool: @pool) else super end ``` but we'd need more changes to trilogy before that is possible.
viralpraxis
pushed a commit
to viralpraxis/rails
that referenced
this pull request
Mar 24, 2024
This commit makes a few changes to our trilogy error translation: * trilogy-libraries/trilogy#118 introduced `Trilogy::EOFError` which we can use instead of matching on `TRILOGY_CLOSED_CONNECTION`. * trilogy-libraries/trilogy#15 introduced `Trilogy::ConnectionClosed`, which inherits from `IOError` for backwards compatibility. As far as I can tell that's the only `IOError` trilogy can raise, so this commit rescues the trilogy-specific error instead. * As far as I can tell Trilogy does not raise `SocketError`, so don't bother translating that * Don't treat TRILOGY_UNEXPECTED_PACKET as a connection error. If we get this, it's probably a bug in trilogy that we should fix. I'd like to eventually get rid of TRILOGY_INVALID_SEQUENCE_ID too, but we're currently relying on it in a few tests (related to trilogy missing caching_sha2_password auth support, if I recall correctly) I'm kinda hoping we'll eventually be able to simplify this to something like: ```rb if exception.is_a?(Trilogy::ConnectionError) ConnectionFailed.new(message, connection_pool: @pool) else super end ``` but we'd need more changes to trilogy before that is possible.
fractaledmind
pushed a commit
to fractaledmind/rails
that referenced
this pull request
May 13, 2024
This commit makes a few changes to our trilogy error translation: * trilogy-libraries/trilogy#118 introduced `Trilogy::EOFError` which we can use instead of matching on `TRILOGY_CLOSED_CONNECTION`. * trilogy-libraries/trilogy#15 introduced `Trilogy::ConnectionClosed`, which inherits from `IOError` for backwards compatibility. As far as I can tell that's the only `IOError` trilogy can raise, so this commit rescues the trilogy-specific error instead. * As far as I can tell Trilogy does not raise `SocketError`, so don't bother translating that * Don't treat TRILOGY_UNEXPECTED_PACKET as a connection error. If we get this, it's probably a bug in trilogy that we should fix. I'd like to eventually get rid of TRILOGY_INVALID_SEQUENCE_ID too, but we're currently relying on it in a few tests (related to trilogy missing caching_sha2_password auth support, if I recall correctly) I'm kinda hoping we'll eventually be able to simplify this to something like: ```rb if exception.is_a?(Trilogy::ConnectionError) ConnectionFailed.new(message, connection_pool: @pool) else super end ``` but we'd need more changes to trilogy before that is possible.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes two of the changes there seems to be consensus around from #113
First this introduces
EOFError, which representsTRILOGY_CLOSED_CONNECTION. It's a subclass ofBaseConnectionError, but distinct fromConnectionClosed(which represents an explicit.closefrom the client's side).Second this makes
Trilogy::SyscallError::*all include theConnectionErrormodule, allowingrescue ConnectionErrorto catch most types of network communication issue.