add timeout to abi request #209
Conversation
WalkthroughThe change updates the HTTP request logic in the Changes
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/common/abi.go (2)
36-58: Consider adding additional timeout contextWhile the 10-second timeout is a good improvement, consider also using a context with timeout for even more robust control over the request lifecycle, especially for long-running operations.
func GetABIForContract(chainId string, contract string) (*abi.ABI, error) { url := fmt.Sprintf("%s/abi/%s/%s", config.Cfg.API.ThirdwebContractApi, chainId, contract) // Create a custom client with timeouts client := &http.Client{ Timeout: 10 * time.Second, } - resp, err := client.Get(url) + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return nil, fmt.Errorf("failed to create request: %v", err) + } + + resp, err := client.Do(req) if err != nil { return nil, fmt.Errorf("failed to get contract abi: %v", err) } defer resp.Body.Close()Don't forget to add
"context"to the imports if you implement this suggestion.
35-58: Consider extracting the timeout value to a configurable constantThe 10-second timeout is hardcoded. For better maintainability and flexibility, consider extracting this value to a constant or configuration parameter.
+const defaultABIRequestTimeout = 10 * time.Second func GetABIForContract(chainId string, contract string) (*abi.ABI, error) { url := fmt.Sprintf("%s/abi/%s/%s", config.Cfg.API.ThirdwebContractApi, chainId, contract) // Create a custom client with timeouts client := &http.Client{ - Timeout: 10 * time.Second, + Timeout: defaultABIRequestTimeout, }Or consider adding it to your configuration structure if you want it to be configurable at runtime.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/common/abi.go(2 hunks)
🔇 Additional comments (2)
internal/common/abi.go (2)
9-9: Added time package for timeout functionalityThe addition of the time package is necessary for setting the HTTP client timeout.
38-43: Good implementation of HTTP client with timeoutAdding a 10-second timeout to the HTTP client is a solid improvement to prevent requests from hanging indefinitely. This helps avoid resource leaks and degraded performance when API endpoints are unresponsive.
The implementation is clean with a helpful comment explaining the purpose.
TL;DR
Added HTTP timeout for ABI fetching operations to prevent hanging requests.
What changed?
timepackage to support the timeout functionalityhttp.Get()call with a custom client that has timeout configurationHow to test?
Why make this change?
Without a timeout, HTTP requests to fetch contract ABIs could potentially hang indefinitely if the API endpoint is unresponsive, causing resource leaks and degraded performance. This change ensures that requests will fail gracefully after 10 seconds, allowing the system to handle errors appropriately and maintain responsiveness.
Summary by CodeRabbit