fix: configure default timeout for blob clients #1191
Conversation
WalkthroughThe changes introduce configurable HTTP client timeouts for several client structs by initializing each with a dedicated Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ClientConstructor
participant HTTPClient
Caller->>ClientConstructor: Instantiate new client (e.g., NewBeaconNodeClient)
ClientConstructor->>HTTPClient: Create http.Client with 15s timeout
ClientConstructor-->>Caller: Return client with configured HTTP client
Caller->>ClientConstructor: Call method (e.g., GetBlobByVersionedHashAndBlockTime)
ClientConstructor->>HTTPClient: Make HTTP request using internal client
HTTPClient-->>ClientConstructor: Return HTTP response (with timeout enforced)
ClientConstructor-->>Caller: Return result
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
params/version.go(1 hunks)rollup/da_syncer/blob_client/beacon_node_client.go(5 hunks)rollup/da_syncer/blob_client/blob_scan_client.go(1 hunks)rollup/da_syncer/blob_client/block_native_client.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
rollup/da_syncer/blob_client/beacon_node_client.go (2)
rollup/l1/types.go (1)
Client(12-23)rollup/da_syncer/blob_client/blob_scan_client.go (1)
BlobScanDefaultTimeout(19-19)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (14)
params/version.go (1)
27-27: LGTM: Appropriate patch version increment.The version bump from 49 to 50 correctly follows semantic versioning for this bug fix that adds timeout configuration to blob clients.
rollup/da_syncer/blob_client/blob_scan_client.go (3)
11-11: LGTM: Necessary import for timeout functionality.The
timeimport is correctly added to support the timeout constant.
18-20: LGTM: Well-defined timeout constant.The 15-second timeout is reasonable for preventing indefinite client hangs while allowing sufficient time for normal API responses.
29-29: LGTM: Proper HTTP client initialization with timeout.The constructor correctly initializes the HTTP client with the defined timeout, replacing reliance on the default client which has no timeout.
rollup/da_syncer/blob_client/block_native_client.go (5)
11-11: LGTM: Necessary import for timeout functionality.The
timeimport is correctly added to support the timeout constant.
18-20: LGTM: Consistent timeout configuration.The 15-second timeout matches other blob clients and provides reasonable protection against indefinite hangs.
23-23: LGTM: Proper client field addition.Adding the HTTP client field to the struct enables timeout configuration for all requests.
29-29: LGTM: Correct client initialization.The constructor properly initializes the HTTP client with the configured timeout.
44-44: LGTM: Using configured client instead of default.The method correctly uses the timeout-configured client instead of the default HTTP client, ensuring requests respect the timeout.
rollup/da_syncer/blob_client/beacon_node_client.go (5)
12-12: LGTM: Necessary import for timeout functionality.The
timeimport is correctly added to support the timeout constant.
18-20: LGTM: Consistent timeout constant definition.The 15-second timeout constant is properly defined and matches other blob clients.
23-23: LGTM: Proper client field addition.Adding the HTTP client field enables timeout configuration for all beacon node requests.
43-43: LGTM: Consistent use of configured HTTP client.All HTTP requests correctly use the timeout-configured client instead of the default client, ensuring proper timeout behavior.
Also applies to: 73-73, 117-117
102-102: LGTM: Proper client field initialization.The client field is correctly assigned in the struct initialization.
1. Purpose or design rationale of this PR
Avoid issues where the client hangs indefinitely, blocking follower node sync.
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Chores