Refactor error classification in shannon: use idiomatic errors.Is/As instead of string matching #503
Conversation
This file contains hidden or 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
…instead of string matching ### Description This PR improves the robustness and performance of error classification in the Shannon protocol package by replacing fragile string-based error detection with idiomatic Go error handling (`errors.Is` and `errors.As`). Key changes: - **Removed string matching** for context errors (timeout/cancel) and HTTP non-2xx errors – now uses `errors.Is` which works correctly with wrapped errors. - **Refactored `isEndpointNetworkConfigError`** to use `errors.As` for precise type matching: - Covers `*net.DNSError`, `*net.OpError` (dial), and common `x509` certificate errors (`HostnameError`, `UnknownAuthorityError`, `CertificateInvalidError`). - Eliminates brittle substring checks that could break with Go version changes or different locales. - **Added missing imports**: `net` and `crypto/x509`. - **Reorganized sentinel error declarations** with clearer grouping and inline comments for better readability. - **Updated comments** to reflect the new robust approach. ### Benefits - More reliable error detection (works with deeply wrapped errors). - Better performance (no unnecessary `.Error()` string allocations). - More maintainable and future-proof (aligns with modern Go best practices). - No behavioral changes – classification semantics remain identical. ### Testing - Existing unit/integration tests should pass unchanged. - Manually verified that common network errors (DNS lookup failure, TLS cert issues) are still classified as `errRelayEndpointConfig`. This cleanup makes the error handling more professional and resilient.
…tion This change replaces brittle string-based error matching in relay error handling with robust, typed classification using errors.Is and errors.As. Key improvements: - Use errors.Is to correctly detect context deadline, cancellation, and wrapped HTTP relay errors. - Use errors.As to classify network and endpoint configuration errors based on concrete types (DNS, net.OpError, x509, TLS), rather than string inspection. - Broaden network error detection to cover all net.OpError operations (dial, read, write, connect), avoiding under-classification. - Add coverage for TLS handshake/record-level errors not represented by x509 certificate types. - Improve maintainability, correctness, and future compatibility with wrapped errors and Go stdlib changes. Behavior is preserved or improved while reducing false positives and ensuring error classification remains stable as error messages evolve.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Description
This PR improves the robustness and performance of error classification in the Shannon protocol package by replacing fragile string-based error detection with idiomatic Go error handling (
errors.Isanderrors.As).Key changes:
errors.Iswhich works correctly with wrapped errors.isEndpointNetworkConfigErrorto useerrors.Asfor precise type matching:*net.DNSError,*net.OpError(dial), and commonx509certificate errors (HostnameError,UnknownAuthorityError,CertificateInvalidError).netandcrypto/x509.Benefits
.Error()string allocations).Testing
errRelayEndpointConfig.This cleanup makes the error handling more professional and resilient.