Add connection timeout #837
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
paulgb
force-pushed
the
paulgb/add-connection-timeout
branch
from
d633deb
to
8f16e3e
Compare
📝 WalkthroughWalkthroughThis pull request introduces several changes to the Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
🧹 Outside diff range and nitpick comments (7)
dynamic-proxy/src/lib.rs (1)
2-2: LGTM! Consider adding module documentation.The addition of the public
connectormodule is well-structured and appropriately placed. The public visibility is correct as it needs to expose the timeout functionality.Consider adding module-level documentation (//! comments) in
connector.rsto describe:
- The purpose and functionality of the TimeoutHttpConnector
- Usage examples
- Any configuration options for timeouts
dynamic-proxy/Cargo.toml (1)
26-27: LGTM! Dependencies align well with the connection timeout implementation.The addition of
tower-serviceandfutures-utilis appropriate for implementing the connection timeout functionality. The versions chosen are stable and widely used.Consider using semantic versioning ranges for better flexibility in minor updates:
-tower-service = "0.3.3" -futures-util = "0.3.30" +tower-service = "0.3" +futures-util = "0.3"dynamic-proxy/src/proxy.rs (3)
17-20: Address the TODO comment for the timeout field.The timeout field is marked as unused but has a default value of 10 seconds. Consider either:
- Implementing the timeout configuration functionality
- Removing the field if it's no longer needed with the new TimeoutHttpConnector
Would you like me to help implement the timeout configuration or open an issue to track this task?
Line range hint
30-34: Consider making the connection timeout configurable.While using
TimeoutHttpConnector::default()works, it would be beneficial to:
- Document the default connection timeout value
- Allow configuration of the timeout duration through the
ProxyClient::new()methodConsider this approach:
- pub fn new() -> Self { + pub fn new() -> Self { + Self::with_timeout(DEFAULT_TIMEOUT) + } + + pub fn with_timeout(connection_timeout: Duration) -> Self { let client = Client::builder(TokioExecutor::new()) - .build(TimeoutHttpConnector::default()); + .build(TimeoutHttpConnector::new(connection_timeout)); Self { client, timeout: DEFAULT_TIMEOUT, } }
Missing error mapping between TimeoutHttpConnectorError and ProxyError
The verification reveals a gap in error handling. While both
TimeoutHttpConnectorErrorandProxyErrorhave appropriate timeout variants, there is noimpl From<TimeoutHttpConnectorError> for ProxyErrorto properly map between them. This means timeout errors from the connector might not be correctly translated toProxyError::Timeout.
- Add an implementation to map
TimeoutHttpConnectorError::TimeouttoProxyError::Timeout- Map
TimeoutHttpConnectorError::BoxedtoProxyError::RequestFailedExample implementation:
impl From<TimeoutHttpConnectorError> for ProxyError { fn from(err: TimeoutHttpConnectorError) -> Self { match err { TimeoutHttpConnectorError::Timeout => ProxyError::Timeout, TimeoutHttpConnectorError::Boxed(e) => ProxyError::RequestFailed(e), } } }🔗 Analysis chain
Line range hint
44-63: Verify error mapping for TimeoutHttpConnector.The error handling looks good, but let's verify that the new
TimeoutHttpConnector's timeout errors are properly mapped toProxyError::Timeout.
Let me gather more information to verify how these connector errors are mapped to ProxyError.
Let me check one more thing to see how ProxyError is defined and if there are any direct uses of these error types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how TimeoutHttpConnector errors are handled # Test: Look for error mapping in the connector implementation ast-grep --pattern 'impl From<$_> for $_Error { $$$ }' # Test: Look for error type definitions and conversions rg -A 5 'TimeoutHttpConnectorError'Length of output: 1728
Script:
#!/bin/bash # Check how TimeoutHttpConnectorError is converted to ProxyError ast-grep --pattern 'impl From<TimeoutHttpConnectorError> for ProxyError { $$$ }' # Look for other places where TimeoutHttpConnectorError is handled rg -A 3 'TimeoutHttpConnectorError::Timeout'Length of output: 351
Script:
#!/bin/bash # Look for ProxyError definition and its variants rg -A 10 'enum ProxyError' # Look for any direct error handling between these types rg '(TimeoutHttpConnectorError|ProxyError)' --type rustLength of output: 2010
dynamic-proxy/src/connector.rs (2)
10-13: Encapsulate struct fields by making them private.To enhance encapsulation and prevent unintended external modification, consider making the
timeoutandconnectorfields private. If external access or modification is necessary, provide appropriate getter and setter methods or builder patterns.
49-56: Avoid boxing errors to preserve error information.Using
Box<dyn std::error::Error + Send + Sync>can obscure specific error types, making debugging more challenging. Instead, consider defining specific error variants inTimeoutHttpConnectorErrorto represent common errors from the underlying connector. This approach improves error transparency and can potentially enhance performance by avoiding dynamic dispatch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
dynamic-proxy/Cargo.toml(1 hunks)dynamic-proxy/src/connector.rs(1 hunks)dynamic-proxy/src/lib.rs(1 hunks)dynamic-proxy/src/proxy.rs(2 hunks)plane/plane-tests/tests/proxy.rs(0 hunks)
💤 Files with no reviewable changes (1)
- plane/plane-tests/tests/proxy.rs
🔇 Additional comments (1)
dynamic-proxy/src/proxy.rs (1)
3-3: LGTM: Import changes align with the new timeout functionality.
The imports correctly bring in the new TimeoutHttpConnector and the legacy Client implementation.
Also applies to: 9-9
We used to have a timeout on the lifespan of the entire request, which also captured an initial connection timeout, but also failed for long-lived connections like large requests. When we removed that timeout, we also removed the ability to time-out a request when the initial TCP connection hangs.
This adds back a timeout that only applies to the initial connection.