[client]: add timeouts to all request kinds + default timeout #367
Conversation
| /// 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.
little annoying with option here.
| let fut = self.transport.send(serde_json::to_string(¬if).map_err(Error::ParseError)?); | ||
| match call_with_maybe_timeout(fut, self.request_timeout).await { | ||
| Ok(Ok(ok)) => Ok(ok), | ||
| Err(_) => Err(Error::RequestTimeout), |
There was a problem hiding this comment.
this is tokio::time::Elapsed
|
|
||
| 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.
this is oneshot::Cancled err in that case the background thread was terminated and we read the error message from the backend.
| 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.
this annoying of boiler plate but I found no good way of having a helper for Result<T>, and T when the receiver is either expects T or Result<T>
on option is to use generic as the http client is using with F: Fut -> Result<Fut::Output, Error> but that became super complicated with lots of nested results.
| @@ -25,17 +28,25 @@ impl HttpClientBuilder { | |||
| self | |||
| } | |||
|
|
|||
| /// Set request timeout (default is 60 seconds). | |||
| /// | |||
| /// None - implies that no timeout is used. | |||
There was a problem hiding this comment.
| /// None - implies that no timeout is used. | |
| /// `None` - no timeout is used. |
It's explicit, not implied :)
| let res = if let Some(dur) = self.request_timeout { | ||
| 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.
I think you can use select! macro for a bit less boilerplate, but it's fine either way.
Co-authored-by: Maciej Hirsz <1096222+maciejhirsz@users.noreply.github.com>
| .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.
I think we can drop the maybe here; call_with_timeout is verbose enough! :)
| @@ -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.
| /// Request timeout | |
| /// Request timeout. Defaults to 60sec. |
* [clients]: use request timeout by-default * add timeout for notif * more feature flag mess * rexport tokio types * Update ws-client/src/client.rs Co-authored-by: Maciej Hirsz <1096222+maciejhirsz@users.noreply.github.com> * Impose a timeout on all requests Variant of #367 This PR takes a more opinionated stance than #367, where timeouts are optional. In this PR I suggest we make a all requests use a timeout and only let users choose the length. * fmt * Address review grumbles * fmt * Use tokio::select! for cleaner code Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com> Co-authored-by: Maciej Hirsz <1096222+maciejhirsz@users.noreply.github.com> Co-authored-by: Maciej Hirsz <hello@maciej.codes>
|
Closing in favour of #406 |
Add default timeout on the all the clients requests to 60 seconds.
The PR itself is !DRY of in a few places.