-
Notifications
You must be signed in to change notification settings - Fork 177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[client]: add timeouts to all request kinds + default timeout #367
Changes from all commits
cd081c7
41061ce
e6fa701
a380de3
07a3f29
adedbcd
163f9df
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -9,13 +9,16 @@ use crate::v2::{ | |||||
use crate::{Error, TEN_MB_SIZE_BYTES}; | ||||||
use async_trait::async_trait; | ||||||
use fnv::FnvHashMap; | ||||||
use futures::Future; | ||||||
use serde::de::DeserializeOwned; | ||||||
use std::sync::atomic::{AtomicU64, Ordering}; | ||||||
use std::time::Duration; | ||||||
|
||||||
/// Http Client Builder. | ||||||
#[derive(Debug)] | ||||||
pub struct HttpClientBuilder { | ||||||
max_request_body_size: u32, | ||||||
request_timeout: Option<Duration>, | ||||||
} | ||||||
|
||||||
impl HttpClientBuilder { | ||||||
|
@@ -25,17 +28,25 @@ impl HttpClientBuilder { | |||||
self | ||||||
} | ||||||
|
||||||
/// Set request timeout (default is 60 seconds). | ||||||
/// | ||||||
/// None - implies that no timeout is used. | ||||||
pub fn request_timeout(mut self, timeout: Option<Duration>) -> Self { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. little annoying with option here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I reckon that's fine. |
||||||
self.request_timeout = timeout; | ||||||
self | ||||||
} | ||||||
|
||||||
/// Build the HTTP client with target to connect to. | ||||||
pub fn build(self, target: impl AsRef<str>) -> Result<HttpClient, Error> { | ||||||
let transport = | ||||||
HttpTransportClient::new(target, self.max_request_body_size).map_err(|e| Error::Transport(Box::new(e)))?; | ||||||
Ok(HttpClient { transport, request_id: AtomicU64::new(0) }) | ||||||
Ok(HttpClient { transport, request_id: AtomicU64::new(0), request_timeout: self.request_timeout }) | ||||||
} | ||||||
} | ||||||
|
||||||
impl Default for HttpClientBuilder { | ||||||
fn default() -> Self { | ||||||
Self { max_request_body_size: TEN_MB_SIZE_BYTES } | ||||||
Self { max_request_body_size: TEN_MB_SIZE_BYTES, request_timeout: Some(Duration::from_secs(60)) } | ||||||
} | ||||||
} | ||||||
|
||||||
|
@@ -46,16 +57,20 @@ pub struct HttpClient { | |||||
transport: HttpTransportClient, | ||||||
/// Request ID that wraps around when overflowing. | ||||||
request_id: AtomicU64, | ||||||
/// Request timeout | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
request_timeout: Option<Duration>, | ||||||
} | ||||||
|
||||||
#[async_trait] | ||||||
impl Client for HttpClient { | ||||||
async fn notification<'a>(&self, method: &'a str, params: JsonRpcParams<'a>) -> Result<(), Error> { | ||||||
let notif = JsonRpcNotificationSer::new(method, params); | ||||||
self.transport | ||||||
.send(serde_json::to_string(¬if).map_err(Error::ParseError)?) | ||||||
.await | ||||||
.map_err(|e| Error::Transport(Box::new(e))) | ||||||
let fut = self.transport.send(serde_json::to_string(¬if).map_err(Error::ParseError)?); | ||||||
match call_with_maybe_timeout(fut, self.request_timeout).await { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can drop the |
||||||
Ok(Ok(ok)) => Ok(ok), | ||||||
Err(_) => Err(Error::RequestTimeout), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is |
||||||
Ok(Err(e)) => Err(Error::Transport(Box::new(e))), | ||||||
} | ||||||
} | ||||||
|
||||||
/// Perform a request towards the server. | ||||||
|
@@ -67,11 +82,12 @@ impl Client for HttpClient { | |||||
let id = self.request_id.fetch_add(1, Ordering::SeqCst); | ||||||
let request = JsonRpcCallSer::new(Id::Number(id), method, params); | ||||||
|
||||||
let body = self | ||||||
.transport | ||||||
.send_and_read_body(serde_json::to_string(&request).map_err(Error::ParseError)?) | ||||||
.await | ||||||
.map_err(|e| Error::Transport(Box::new(e)))?; | ||||||
let fut = self.transport.send_and_read_body(serde_json::to_string(&request).map_err(Error::ParseError)?); | ||||||
let body = match call_with_maybe_timeout(fut, self.request_timeout).await { | ||||||
Ok(Ok(body)) => body, | ||||||
Err(_e) => return Err(Error::RequestTimeout), | ||||||
Ok(Err(e)) => return Err(Error::Transport(Box::new(e))), | ||||||
}; | ||||||
|
||||||
let response: JsonRpcResponse<_> = match serde_json::from_slice(&body) { | ||||||
Ok(response) => response, | ||||||
|
@@ -106,11 +122,13 @@ impl Client for HttpClient { | |||||
request_set.insert(id, pos); | ||||||
} | ||||||
|
||||||
let body = self | ||||||
.transport | ||||||
.send_and_read_body(serde_json::to_string(&batch_request).map_err(Error::ParseError)?) | ||||||
.await | ||||||
.map_err(|e| Error::Transport(Box::new(e)))?; | ||||||
let fut = self.transport.send_and_read_body(serde_json::to_string(&batch_request).map_err(Error::ParseError)?); | ||||||
|
||||||
let body = match call_with_maybe_timeout(fut, self.request_timeout).await { | ||||||
Ok(Ok(body)) => body, | ||||||
Err(_e) => return Err(Error::RequestTimeout), | ||||||
Ok(Err(e)) => return Err(Error::Transport(Box::new(e))), | ||||||
}; | ||||||
|
||||||
let rps: Vec<JsonRpcResponse<_>> = match serde_json::from_slice(&body) { | ||||||
Ok(response) => response, | ||||||
|
@@ -133,3 +151,14 @@ impl Client for HttpClient { | |||||
Ok(responses) | ||||||
} | ||||||
} | ||||||
|
||||||
async fn call_with_maybe_timeout<F>(fut: F, timeout: Option<Duration>) -> Result<F::Output, crate::tokio::Elapsed> | ||||||
where | ||||||
F: Future, | ||||||
{ | ||||||
if let Some(dur) = timeout { | ||||||
crate::tokio::timeout(dur, fut).await | ||||||
} else { | ||||||
Ok(fut.await) | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -185,7 +185,7 @@ impl<'a> Default for WsClientBuilder<'a> { | |
Self { | ||
certificate_store: CertificateStore::Native, | ||
max_request_body_size: TEN_MB_SIZE_BYTES, | ||
request_timeout: None, | ||
request_timeout: Some(Duration::from_secs(60)), | ||
connection_timeout: Duration::from_secs(10), | ||
origin_header: None, | ||
max_concurrent_requests: 256, | ||
|
@@ -207,9 +207,11 @@ impl<'a> WsClientBuilder<'a> { | |
self | ||
} | ||
|
||
/// Set request timeout. | ||
pub fn request_timeout(mut self, timeout: Duration) -> Self { | ||
self.request_timeout = Some(timeout); | ||
/// Set request timeout (default is 60 seconds). | ||
/// | ||
/// None - no timeout is used. | ||
pub fn request_timeout(mut self, timeout: Option<Duration>) -> Self { | ||
self.request_timeout = timeout; | ||
self | ||
} | ||
|
||
|
@@ -312,7 +314,21 @@ impl Client for WsClient { | |
Error::ParseError(e) | ||
})?; | ||
log::trace!("[frontend]: send notification: {:?}", raw); | ||
let res = self.to_back.clone().send(FrontToBack::Notification(raw)).await; | ||
|
||
let mut sender = self.to_back.clone(); | ||
let fut = sender.send(FrontToBack::Notification(raw)); | ||
|
||
let res = if let Some(dur) = self.request_timeout { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this annoying of boiler plate but I found no good way of having a helper for on option is to use generic as the |
||
let timeout = crate::tokio::sleep(dur); | ||
futures::pin_mut!(fut, timeout); | ||
match futures::future::select(fut, timeout).await { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can use |
||
futures::future::Either::Left((res, _)) => res, | ||
futures::future::Either::Right((_, _)) => return Err(Error::RequestTimeout), | ||
} | ||
} else { | ||
fut.await | ||
}; | ||
|
||
self.id_guard.reclaim_request_id(); | ||
match res { | ||
Ok(()) => Ok(()), | ||
|
@@ -343,19 +359,10 @@ impl Client for WsClient { | |
return Err(self.read_error_from_backend().await); | ||
} | ||
|
||
let send_back_rx_out = if let Some(duration) = self.request_timeout { | ||
let timeout = crate::tokio::sleep(duration); | ||
futures::pin_mut!(send_back_rx, timeout); | ||
match future::select(send_back_rx, timeout).await { | ||
future::Either::Left((send_back_rx_out, _)) => send_back_rx_out, | ||
future::Either::Right((_, _)) => Ok(Err(Error::RequestTimeout)), | ||
} | ||
} else { | ||
send_back_rx.await | ||
}; | ||
let res = crate::helpers::call_with_maybe_timeout(send_back_rx, self.request_timeout).await; | ||
|
||
self.id_guard.reclaim_request_id(); | ||
let json_value = match send_back_rx_out { | ||
let json_value = match res { | ||
Ok(Ok(v)) => v, | ||
Ok(Err(err)) => return Err(err), | ||
Err(_) => return Err(self.read_error_from_backend().await), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is |
||
|
@@ -392,7 +399,8 @@ impl Client for WsClient { | |
return Err(self.read_error_from_backend().await); | ||
} | ||
|
||
let res = send_back_rx.await; | ||
let res = crate::helpers::call_with_maybe_timeout(send_back_rx, self.request_timeout).await; | ||
|
||
self.id_guard.reclaim_request_id(); | ||
let json_values = match res { | ||
Ok(Ok(v)) => v, | ||
|
@@ -452,7 +460,8 @@ impl SubscriptionClient for WsClient { | |
return Err(self.read_error_from_backend().await); | ||
} | ||
|
||
let res = send_back_rx.await; | ||
let res = crate::helpers::call_with_maybe_timeout(send_back_rx, self.request_timeout).await; | ||
|
||
self.id_guard.reclaim_request_id(); | ||
let (notifs_rx, id) = match res { | ||
Ok(Ok(val)) => val, | ||
|
@@ -483,7 +492,8 @@ impl SubscriptionClient for WsClient { | |
return Err(self.read_error_from_backend().await); | ||
} | ||
|
||
let res = send_back_rx.await; | ||
let res = crate::helpers::call_with_maybe_timeout(send_back_rx, self.request_timeout).await; | ||
|
||
let (notifs_rx, method) = match res { | ||
Ok(Ok(val)) => val, | ||
Ok(Err(err)) => return Err(err), | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's explicit, not implied :)