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

Raise ActiveResource::ConnectionRefusedError on Errno::ECONNREFUSED #396

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

balvig
Copy link
Contributor

@balvig balvig commented Dec 12, 2023

What

Raises a custom ActiveResource::ConnectionError when failing to connect to an external endpoint completely, changing something like:

Errno::ECONNREFUSED: Failed to open TCP connection to 127.0.0.1:4568 (Connection refused - connect(2) for "127.0.0.1" port 4568)

...into:

ActiveResource::ConnectionRefusedError: Failed to open TCP connection to 127.0.0.1:4568 (Connection refused - connect(2) for "127.0.0.1" port 4568)

Why

Follows the convention of rescuing network errors and wrapping them in ActiveResource custom errors, allowing them to be rescued without leaking knowledge of internals.

@balvig balvig marked this pull request as ready for review December 12, 2023 01:48
@balvig balvig changed the title Raise ActiveResource::ConnectionError on Errno::ECONNREFUSED Raise ActiveResource::ConnectionRefusedError on Errno::ECONNREFUSED Dec 12, 2023
@rafaelfranca
Copy link
Member

This is a behavior change. If people are already rescuing ECONNREFUSED they would now get error in production. Can we do it without breaking existing code?

@balvig
Copy link
Contributor Author

balvig commented Jan 29, 2024

Cheers, good shout @rafaelfranca!

The only thing that immediately comes to mind is having the custom exception inherit from ECONNREFUSED rather than ConnectionError.

The lack of parity with the other custom exceptions feels a little off, but it might be worth it to avoid a breaking change?
Am I otherwise missing something? 🤔

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.

2 participants