sdk: introduce RetryKind next to the HttpError to determine errors that should be retried

retry kind: make `characterize_retry_kind` a method of `HttpError`

---

retry kind: mark all network errors as transient

This should cause more backoff in general, if the client is disconnected
of the grid, or the remote server is, which is good behavior to have in
general.

---

http error: introduce another rustfmt-formated impl block for HttpError

The previous `impl` block was marked as skipping rustfmt, which led to
weird formatting behavior.

Changes are formatting only.

Author: bnjbvr

crates/matrix-sdk/src/error.rs

@@ -14,7 +14,7 @@
 
 //! Error conditions.
 
-use std::{io::Error as IoError, sync::Arc};
+use std::{io::Error as IoError, sync::Arc, time::Duration};
 
 use as_variant::as_variant;
 #[cfg(feature = "qrcode")]
@@ -122,13 +122,15 @@ impl HttpError {
     pub fn as_client_api_error(&self) -> Option<&ruma::api::client::Error> {
         self.as_ruma_api_error().and_then(RumaApiError::as_client_api_error)
     }
+}
 
+// Another impl block that's formatted with rustfmt.
+impl HttpError {
     /// If `self` is a server error in the `errcode` + `error` format expected
     /// for client-API endpoints, returns the error kind (`errcode`).
     pub fn client_api_error_kind(&self) -> Option<&ErrorKind> {
-        self.as_client_api_error().and_then(|e| {
-            as_variant!(&e.body, ErrorBody::Standard { kind, .. } => kind)
-        })
+        self.as_client_api_error()
+            .and_then(|e| as_variant!(&e.body, ErrorBody::Standard { kind, .. } => kind))
     }
 
     /// Try to destructure the error into an universal interactive auth info.
@@ -145,6 +147,59 @@ impl HttpError {
     pub fn as_uiaa_response(&self) -> Option<&UiaaInfo> {
         self.as_ruma_api_error().and_then(as_variant!(RumaApiError::Uiaa))
     }
+
+    /// Returns whether an HTTP error response should be qualified as transient
+    /// or permanent.
+    pub(crate) fn retry_kind(&self) -> RetryKind {
+        use ruma::api::client::error::{ErrorBody, ErrorKind, RetryAfter};
+
+        use crate::RumaApiError;
+
+        // If it was a plain network error, it's either that we're disconnected from the
+        // internet, or that the remote is, so retry a few times.
+        if matches!(self, Self::Reqwest(..)) {
+            return RetryKind::Transient { retry_after: None };
+        }
+
+        if let Some(api_error) = self.as_ruma_api_error() {
+            let status_code = match api_error {
+                RumaApiError::ClientApi(e) => match e.body {
+                    ErrorBody::Standard {
+                        kind: ErrorKind::LimitExceeded { retry_after }, ..
+                    } => {
+                        let retry_after = retry_after.and_then(|retry_after| match retry_after {
+                            RetryAfter::Delay(d) => Some(d),
+                            RetryAfter::DateTime(_) => None,
+                        });
+                        return RetryKind::Transient { retry_after };
+                    }
+                    _ => Some(e.status_code),
+                },
+                RumaApiError::Uiaa(_) => None,
+                RumaApiError::Other(e) => Some(e.status_code),
+            };
+
+            if let Some(status_code) = status_code {
+                if status_code.is_server_error() {
+                    return RetryKind::Transient { retry_after: None };
+                }
+            }
+        }
+
+        RetryKind::Permanent
+    }
+}
+
+/// How should we behave with respect to retry behavior after an `HttpError`
+/// happened?
+pub(crate) enum RetryKind {
+    Transient {
+        // This is used only for attempts to retry, so on non-wasm32 code (in the `native` module).
+        #[cfg_attr(target_arch = "wasm32", allow(dead_code))]
+        retry_after: Option<Duration>,
+    },
+
+    Permanent,
 }
 
 /// Internal representation of errors.

crates/matrix-sdk/src/http_client/mod.rs

@@ -259,56 +259,3 @@ impl tower::Service<http_old::Request<Bytes>> for HttpClient {
         Box::pin(fut)
     }
 }
-
-pub(crate) enum RetryKind {
-    Transient {
-        err: HttpError,
-        // This is used only for attempts to retry, so on non-wasm32 code (in the `native` module).
-        #[cfg_attr(target_arch = "wasm32", allow(dead_code))]
-        retry_after: Option<Duration>,
-    },
-    Permanent(HttpError),
-}
-
-impl RetryKind {
-    pub fn error(self) -> HttpError {
-        match self {
-            RetryKind::Transient { err, .. } | RetryKind::Permanent(err) => err,
-        }
-    }
-}
-
-/// Returns whether an HTTP error response should be qualified as transient or
-/// permanent.
-///
-/// This is what decides whether to retry requests, in the [`native`] module.
-pub(crate) fn characterize_retry_kind(err: HttpError) -> RetryKind {
-    use ruma::api::client::error::{ErrorBody, ErrorKind, RetryAfter};
-
-    use crate::RumaApiError;
-
-    if let Some(api_error) = err.as_ruma_api_error() {
-        let status_code = match api_error {
-            RumaApiError::ClientApi(e) => match e.body {
-                ErrorBody::Standard { kind: ErrorKind::LimitExceeded { retry_after }, .. } => {
-                    let retry_after = retry_after.and_then(|retry_after| match retry_after {
-                        RetryAfter::Delay(d) => Some(d),
-                        RetryAfter::DateTime(_) => None,
-                    });
-                    return RetryKind::Transient { err, retry_after };
-                }
-                _ => Some(e.status_code),
-            },
-            RumaApiError::Uiaa(_) => None,
-            RumaApiError::Other(e) => Some(e.status_code),
-        };
-
-        if let Some(status_code) = status_code {
-            if status_code.is_server_error() {
-                return RetryKind::Transient { err, retry_after: None };
-            }
-        }
-    }
-
-    RetryKind::Permanent(err)
-}

crates/matrix-sdk/src/http_client/native.rs

@@ -28,11 +28,11 @@ use reqwest::Certificate;
 use ruma::api::{error::FromHttpResponseError, IncomingResponse, OutgoingRequest};
 use tracing::{info, warn};
 
-use super::{
-    characterize_retry_kind, response_to_http_response, HttpClient, RetryKind,
-    TransmissionProgress, DEFAULT_REQUEST_TIMEOUT,
+use super::{response_to_http_response, HttpClient, TransmissionProgress, DEFAULT_REQUEST_TIMEOUT};
+use crate::{
+    config::RequestConfig,
+    error::{HttpError, RetryKind},
 };
-use crate::{config::RequestConfig, error::HttpError};
 
 impl HttpClient {
     pub(super) async fn send_request<R>(
@@ -62,11 +62,11 @@ impl HttpClient {
                 let error_type = if stop {
                     RetryError::Permanent
                 } else {
-                    |err: HttpError| match characterize_retry_kind(err) {
-                        RetryKind::Transient { err, retry_after } => {
+                    |err: HttpError| match err.retry_kind() {
+                        RetryKind::Transient { retry_after } => {
                             RetryError::Transient { err, retry_after }
                         }
-                        RetryKind::Permanent(err) => RetryError::Permanent(err),
+                        RetryKind::Permanent => RetryError::Permanent(err),
                     }
                 };
 

crates/matrix-sdk/src/send_queue.rs

@@ -32,11 +32,7 @@ use tokio::sync::{broadcast, Notify, RwLock};
 use tracing::{debug, error, info, instrument, trace, warn};
 
 use crate::{
-    client::WeakClient,
-    config::RequestConfig,
-    http_client::{characterize_retry_kind, RetryKind},
-    room::WeakRoom,
-    Client, Room,
+    client::WeakClient, config::RequestConfig, error::RetryKind, room::WeakRoom, Client, Room,
 };
 
 /// A client-wide send queue, for all the rooms known by a client.
@@ -366,30 +362,11 @@ impl RoomSendQueue {
                 }
 
                 Err(err) => {
-                    let (err, is_recoverable) = if let crate::Error::Http(http_err) = err {
-                        // A small trick to confuse borrowck (and humans too).
-                        //
-                        // `is_transient_error` really wants ownership of the error, so we can't
-                        // match above on `&err`, and we have to pass the error around by ownership
-                        // all over the place. We give ownership in `is_transient_error`, and then
-                        // get it back with the below call to `.error()`. Oh well.
-                        let retry_kind = characterize_retry_kind(http_err);
-
-                        let mut is_recoverable = matches!(retry_kind, RetryKind::Transient { .. });
-
-                        let http_err = retry_kind.error();
-
-                        if !is_recoverable {
-                            // Also flag all HTTP errors as recoverable: they might indicate that
-                            // the server was unreachable, or that the device is entirely
-                            // disconnected.
-                            is_recoverable =
-                                matches!(http_err, crate::error::HttpError::Reqwest(..));
-                        }
-
-                        (crate::Error::Http(http_err), is_recoverable)
+                    let is_recoverable = if let crate::Error::Http(ref http_err) = err {
+                        // All transient errors are recoverable.
+                        matches!(http_err.retry_kind(), RetryKind::Transient { .. })
                     } else {
-                        (err, false)
+                        false
                     };
 
                     if is_recoverable {