Skip to content

Conversation

@maximus1108
Copy link
Contributor

@maximus1108 maximus1108 commented Feb 4, 2025

What?

  • Catch and raise EHOSTUNREACH and EADDRNOTAVAIL syscall errors as ActiveUtils::ConnectionError
  • Release as version 3.5.0

Why?

Resolves https://github.com/Shopify/shopify/issues/576905 and similar issues

EHOSTUNREACH is returned when a host is unreachable due to network conditions such as firewall rules. EADDRNOTAVAIL is returned when the host is address is not valid - this specifically happens in Core when for hosts such as localhost which are not permitted.

Although not highly prevalent, it does create noise among our exceptions and these errors are not exceptional. Below are some rough numbers on these errors (query)
image

We should raise these as ActiveUtils::ConnectionError because they're generally they're not transient and retries would be ineffective. This is also helpful for existing users of this library because most are already handling ActiveUtils::ConnectionError appropriately

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

@maximus1108 maximus1108 force-pushed the connection-err-for-ehostunreach-and-eaddrnot-avail branch from cee4e2a to 9ff93f9 Compare February 4, 2025 10:16
Copy link
Member

@jbalsas jbalsas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me!

Copy link

@nwpan nwpan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for making this change. 🙏

Copy link

@luanpy luanpy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL about the two errors!

@maximus1108 maximus1108 force-pushed the connection-err-for-ehostunreach-and-eaddrnot-avail branch from 9ff93f9 to 3847f68 Compare February 5, 2025 11:31
@maximus1108 maximus1108 merged commit e1737f1 into main Feb 5, 2025
6 checks passed
@maximus1108 maximus1108 mentioned this pull request Feb 5, 2025
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.

4 participants