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

[Code red] handle HOSTUNREACH and EADDRNOTAVAIL syscall errors (version 3.5.0) #135

Merged

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

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