feat(rpc): add configurable timeouts for JSON-RPC client - #684
Conversation
- Introduced environment variables for total request timeout and TCP connect timeout, allowing customization of the RPC client's behavior. - Implemented a new function to build the RPC client with the specified timeouts. - Updated `RpcBlockDataProvider` to utilize the new RPC client setup. - Added tests to verify timeout behavior in the RPC client.
There was a problem hiding this comment.
Pull request overview
Adds configurable HTTP timeouts to the Alloy/Reqwest-backed JSON-RPC client used by RpcBlockDataProvider, allowing operators to tune overall request and TCP connect behavior via environment variables.
Changes:
- Introduced
RAIKO_RPC_HTTP_TIMEOUT_SECSandRAIKO_RPC_HTTP_CONNECT_TIMEOUT_SECSto configure per-request and connect timeouts. - Centralized RPC HTTP transport construction via
build_rpc_reqwest_client()/rpc_http_transport(). - Added a timeout-focused test for stalled JSON-RPC responses.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn set_short_rpc_timeouts_for_test() { | ||
| std::env::set_var(ENV_RPC_HTTP_TIMEOUT_SECS, "1"); | ||
| std::env::set_var(ENV_RPC_HTTP_CONNECT_TIMEOUT_SECS, "1"); | ||
| } | ||
|
|
||
| fn clear_rpc_timeout_env() { | ||
| std::env::remove_var(ENV_RPC_HTTP_TIMEOUT_SECS); | ||
| std::env::remove_var(ENV_RPC_HTTP_CONNECT_TIMEOUT_SECS); | ||
| } |
There was a problem hiding this comment.
The test mutates global process env vars but clear_rpc_timeout_env() unconditionally removes them (and won’t run if the test panics before line 446). Capture previous values and restore them via a guard (Drop) so existing RAIKO_RPC_HTTP_* settings aren’t lost and cleanup is guaranteed.
| assert!( | ||
| lower.contains("timeout") | ||
| || lower.contains("timed out") | ||
| || payload.contains("error sending request for url"), |
There was a problem hiding this comment.
In the assertion, you lowercase payload into lower but the third branch still checks payload.contains("error sending request for url") case-sensitively. This can make the test flaky across reqwest error message variants/capitalization; use the lowercased string for that check too (or match on error kind if available).
| || payload.contains("error sending request for url"), | |
| || lower.contains("error sending request for url"), |
| let connect_secs: u64 = env::var(ENV_RPC_HTTP_CONNECT_TIMEOUT_SECS) | ||
| .ok() | ||
| .and_then(|s| s.parse().ok()) | ||
| .unwrap_or(30); | ||
|
|
||
| Client::builder() | ||
| .timeout(Duration::from_secs(timeout_secs)) | ||
| .connect_timeout(Duration::from_secs(connect_secs)) | ||
| .build() |
There was a problem hiding this comment.
New behavior adds a separate TCP connect_timeout (driven by RAIKO_RPC_HTTP_CONNECT_TIMEOUT_SECS), but the added test only exercises the overall request timeout. Consider adding a focused test that verifies the connect timeout path (e.g., dial an unroutable IP/port) so regressions in connect-timeout handling are caught.
|
duplicated |
RpcBlockDataProviderto utilize the new RPC client setup.