Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle EADDRNOTAVAIL as a BaseConnectionError instead of SyscallError #98

Closed

Conversation

dhruvCW
Copy link

@dhruvCW dhruvCW commented Jul 10, 2023

instead of letting EADDRNOTAVAIL be handled as a generic SyscallError convert it to a BaseConnectionError instead, in this case we treat it as a a TRILOGY_DNS_ERROR so that the activerecord adapters handle it as a DBConnectionError.

fixes trilogy-libraries/activerecord-trilogy-adapter#52

@dhruvCW dhruvCW changed the title Handle EADDRNOTAVAIL as a BaseConnectionError Handle EADDRNOTAVAIL as a BaseConnectionError instead of SyscallError Jul 10, 2023
instead of letting EADDRNOTAVAIL be handled as a generic SyscallError convert it to a BaseConnectionError
instead, in this case we treat it as a a TRILOGY_DNS_ERROR
so that the activerecord adapters handle it as a DBConnectionError.
@dhruvCW dhruvCW force-pushed the handle_address_unavailable_error branch from c3f6565 to bcf974b Compare July 10, 2023 11:28
@jhawthorn
Copy link
Member

I'm not sure I agree with the motivation here. A Trilogy::SyscallError is what will be raised here and what we'll raise for a number of other connection-related errors. It seems best to use the most specific error we can for the error we've encountered.

@dhruvCW
Copy link
Author

dhruvCW commented Aug 3, 2023

I'm not sure I agree with the motivation here. A Trilogy::SyscallError is what will be raised here and what we'll raise for a number of other connection-related errors. It seems best to use the most specific error we can for the error we've encountered.

don't you think it would make sense to raise an exception that is derived from a Trilogy::ConnectionError base ? I am thinking in terms of a use case like active_record adapter where it might be better to handle all connection errors rather than specific ones ?

In this case though I am not particularly invested anymore since it arises from an invalid system setup and thus is more of an outlier than other errors.

@jhawthorn
Copy link
Member

I think it could make sense as a Trilogy::ConnectionError, but only if that was also done to the other Trilogy::SyscallError::* classes, which are effectively all "connection-level" errors.

thoughts @adrianna-chang-shopify @casperisfine ?

@casperisfine
Copy link
Contributor

thoughts

Yeah, that's why I don't like the syscall errors, I'd like to be able to rescue Trilogy::BaseConnectionError or something, and inheriting from SystemCallError prevents that :/

@adrianna-chang-shopify
Copy link
Collaborator

Yeah, that's why I don't like the syscall errors, I'd like to be able to rescue Trilogy::BaseConnectionError

Yeah, I guess the idea with our approach was that consumers could generically rescue syscall errors as Trilogy::Error, but also use the raw syscall errs (e.g. ECONNRESET) for finer-tuned handling. Changing the syscall errors to include Trilogy::BaseConnectionError makes sense to me 👍

@dhruvCW
Copy link
Author

dhruvCW commented Aug 23, 2023

closing in favour of #118

@dhruvCW dhruvCW closed this Aug 23, 2023
@dhruvCW dhruvCW deleted the handle_address_unavailable_error branch August 23, 2023 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: Trilogy::SyscallError::EADDRNOTAVAIL no longer converted to ActiveRecord::StatementInvalid
4 participants