Merge branch 'urlsession-retry-strategy'

pinkisemils · pinkisemils · commit ad9cb78ccac3 · 2025-03-03T14:24:05.000+01:00
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/ios/MullvadREST/ApiHandlers/MullvadApiRequestFactory.swift b/ios/MullvadREST/ApiHandlers/MullvadApiRequestFactory.swift
@@ -10,7 +10,7 @@ import MullvadRustRuntime
 import MullvadTypes
 
 enum MullvadApiRequest {
-    case getAddressList
+    case getAddressList(retryStrategy: REST.RetryStrategy)
 }
 
 struct MullvadApiRequestFactory {
@@ -25,8 +25,12 @@ struct MullvadApiRequestFactory {
             let rawPointer = Unmanaged.passRetained(pointerClass).toOpaque()
 
             return switch request {
-            case .getAddressList:
-                MullvadApiCancellable(handle: mullvad_api_get_addresses(apiContext.context, rawPointer))
+            case let .getAddressList(retryStrategy):
+                MullvadApiCancellable(handle: mullvad_api_get_addresses(
+                    apiContext.context,
+                    rawPointer,
+                    retryStrategy.toRustStrategy()
+                ))
             }
         }
     }
diff --git a/ios/MullvadREST/ApiHandlers/RESTAPIProxy.swift b/ios/MullvadREST/ApiHandlers/RESTAPIProxy.swift
@@ -66,7 +66,7 @@ extension REST {
             retryStrategy: REST.RetryStrategy,
             completionHandler: @escaping @Sendable ProxyCompletionHandler<[AnyIPEndpoint]>
         ) -> Cancellable {
-            let requestHandler = mullvadApiRequestFactory.makeRequest(.getAddressList)
+            let requestHandler = mullvadApiRequestFactory.makeRequest(.getAddressList(retryStrategy: retryStrategy))
 
             let responseHandler = rustResponseHandler(
                 decoding: [AnyIPEndpoint].self,
@@ -76,7 +76,6 @@ extension REST {
             let networkOperation = MullvadApiNetworkOperation(
                 name: "get-api-addrs",
                 dispatchQueue: dispatchQueue,
-                retryStrategy: retryStrategy,
                 requestHandler: requestHandler,
                 responseDecoder: responseDecoder,
                 responseHandler: responseHandler,
diff --git a/ios/MullvadREST/ApiHandlers/RESTRustNetworkOperation.swift b/ios/MullvadREST/ApiHandlers/RESTRustNetworkOperation.swift
@@ -21,22 +21,14 @@ extension REST {
         private let responseHandler: any RESTRustResponseHandler<Success>
         private var networkTask: MullvadApiCancellable?
 
-        private let retryStrategy: RetryStrategy
-        private var retryDelayIterator: AnyIterator<Duration>
-        private var retryTimer: DispatchSourceTimer?
-        private var retryCount = 0
-
         init(
             name: String,
             dispatchQueue: DispatchQueue,
-            retryStrategy: RetryStrategy,
             requestHandler: @escaping MullvadApiRequestHandler,
             responseDecoder: JSONDecoder,
             responseHandler: some RESTRustResponseHandler<Success>,
             completionHandler: CompletionHandler? = nil
         ) {
-            self.retryStrategy = retryStrategy
-            retryDelayIterator = retryStrategy.makeDelayIterator()
             self.responseDecoder = responseDecoder
             self.requestHandler = requestHandler
             self.responseHandler = responseHandler
@@ -53,10 +45,7 @@ extension REST {
         }
 
         override public func operationDidCancel() {
-            retryTimer?.cancel()
             networkTask?.cancel()
-
-            retryTimer = nil
             networkTask = nil
         }
 
@@ -76,12 +65,7 @@ extension REST {
                 guard let self else { return }
 
                 if let error = response.restError() {
-                    if response.shouldRetry {
-                        retryRequest(with: error)
-                    } else {
-                        finish(result: .failure(error))
-                    }
-
+                    finish(result: .failure(error))
                     return
                 }
 
@@ -97,51 +81,6 @@ extension REST {
                 }
             }
         }
-
-        private func retryRequest(with error: REST.Error) {
-            // Check if retry count is not exceeded.
-            guard retryCount < retryStrategy.maxRetryCount else {
-                if retryStrategy.maxRetryCount > 0 {
-                    logger.debug("Ran out of retry attempts (\(retryStrategy.maxRetryCount))")
-                }
-                finish(result: .failure(error))
-                return
-            }
-
-            // Increment retry count.
-            retryCount += 1
-
-            // Retry immediately if retry delay is set to never.
-            guard retryStrategy.delay != .never else {
-                startRequest()
-                return
-            }
-
-            guard let waitDelay = retryDelayIterator.next() else {
-                logger.debug("Retry delay iterator failed to produce next value.")
-
-                finish(result: .failure(error))
-                return
-            }
-
-            logger.debug("Retry in \(waitDelay.logFormat()).")
-
-            // Create timer to delay retry.
-            let timer = DispatchSource.makeTimerSource(queue: dispatchQueue)
-
-            timer.setEventHandler { [weak self] in
-                self?.startRequest()
-            }
-
-            timer.setCancelHandler { [weak self] in
-                self?.finish(result: .failure(OperationError.cancelled))
-            }
-
-            timer.schedule(wallDeadline: .now() + waitDelay.timeInterval)
-            timer.activate()
-
-            retryTimer = timer
-        }
     }
 }
 
diff --git a/ios/MullvadREST/RetryStrategy/RetryStrategy.swift b/ios/MullvadREST/RetryStrategy/RetryStrategy.swift
@@ -7,6 +7,7 @@
 //
 
 import Foundation
+import MullvadRustRuntime
 import MullvadTypes
 
 extension REST {
@@ -21,6 +22,23 @@ extension REST {
             self.applyJitter = applyJitter
         }
 
+        /// The return value of this function *must* be passed to a Rust FFI function that will consume it, otherwise it will leak.
+        public func toRustStrategy() -> SwiftRetryStrategy {
+            switch delay {
+            case .never:
+                return mullvad_api_retry_strategy_never()
+            case let .constant(duration):
+                return mullvad_api_retry_strategy_constant(UInt(maxRetryCount), UInt64(duration.seconds))
+            case let .exponentialBackoff(initial, multiplier, maxDelay):
+                return mullvad_api_retry_strategy_exponential(
+                    UInt(maxRetryCount),
+                    UInt64(initial.seconds),
+                    UInt32(multiplier),
+                    UInt64(maxDelay.seconds)
+                )
+            }
+        }
+
         public func makeDelayIterator() -> AnyIterator<Duration> {
             let inner = delay.makeIterator()
 
diff --git a/ios/MullvadRustRuntime/MullvadApiResponse.swift b/ios/MullvadRustRuntime/MullvadApiResponse.swift
@@ -45,10 +45,6 @@ public class MullvadApiResponse {
         }
     }
 
-    public var shouldRetry: Bool {
-        response.should_retry
-    }
-
     public var success: Bool {
         response.success
     }
diff --git a/ios/MullvadRustRuntime/include/mullvad_rust_runtime.h b/ios/MullvadRustRuntime/include/mullvad_rust_runtime.h
@@ -26,6 +26,8 @@ typedef struct ExchangeCancelToken ExchangeCancelToken;
 
 typedef struct RequestCancelHandle RequestCancelHandle;
 
+typedef struct RetryStrategy RetryStrategy;
+
 typedef struct SwiftApiContext {
   const struct ApiContext *_0;
 } SwiftApiContext;
@@ -34,15 +36,17 @@ typedef struct SwiftCancelHandle {
   struct RequestCancelHandle *ptr;
 } SwiftCancelHandle;
 
+typedef struct SwiftRetryStrategy {
+  struct RetryStrategy *_0;
+} SwiftRetryStrategy;
+
 typedef struct SwiftMullvadApiResponse {
   uint8_t *body;
   uintptr_t body_size;
   uint16_t status_code;
   uint8_t *error_description;
   uint8_t *server_response_code;
   bool success;
-  bool should_retry;
-  uint64_t retry_after;
 } SwiftMullvadApiResponse;
 
 typedef struct CompletionCookie {
@@ -106,7 +110,8 @@ struct SwiftApiContext mullvad_api_init_new(const uint8_t *host,
  * This function is not safe to call multiple times with the same `CompletionCookie`.
  */
 struct SwiftCancelHandle mullvad_api_get_addresses(struct SwiftApiContext api_context,
-                                                   void *completion_cookie);
+                                                   void *completion_cookie,
+                                                   struct SwiftRetryStrategy retry_strategy);
 
 /**
  * Called by the Swift side to signal that a Mullvad API call should be cancelled.
@@ -156,6 +161,29 @@ extern void mullvad_api_completion_finish(struct SwiftMullvadApiResponse respons
  */
 void mullvad_response_drop(struct SwiftMullvadApiResponse response);
 
+/**
+ * Creates a retry strategy that never retries after failure.
+ * The result needs to be consumed.
+ */
+struct SwiftRetryStrategy mullvad_api_retry_strategy_never(void);
+
+/**
+ * Creates a retry strategy that retries `max_retries` times with a constant delay of `delay_sec`.
+ * The result needs to be consumed.
+ */
+struct SwiftRetryStrategy mullvad_api_retry_strategy_constant(uintptr_t max_retries,
+                                                              uint64_t delay_sec);
+
+/**
+ * Creates a retry strategy that retries `max_retries` times with a exponantially increating delay.
+ * The delay will never exceed `max_delay_sec`
+ * The result needs to be consumed.
+ */
+struct SwiftRetryStrategy mullvad_api_retry_strategy_exponential(uintptr_t max_retries,
+                                                                 uint64_t initial_sec,
+                                                                 uint32_t factor,
+                                                                 uint64_t max_delay_sec);
+
 /**
  * Initializes a valid pointer to an instance of `EncryptedDnsProxyState`.
  *
diff --git a/ios/MullvadTypes/Duration+Extensions.swift b/ios/MullvadTypes/Duration+Extensions.swift
@@ -14,6 +14,10 @@ extension Duration {
         return timeInterval.isFinite
     }
 
+    public var seconds: Int64 {
+        return components.seconds
+    }
+
     public var timeInterval: TimeInterval {
         return TimeInterval(components.seconds) + (TimeInterval(components.attoseconds) * 1e-18)
     }
diff --git a/mullvad-ios/Cargo.toml b/mullvad-ios/Cargo.toml
@@ -24,6 +24,7 @@ hyper-util = { workspace = true }
 tower = { workspace = true }
 tunnel-obfuscation = { path = "../tunnel-obfuscation" }
 oslog = "0.2"
+talpid-future = { path = "../talpid-future" }
 talpid-types = { path = "../talpid-types" }
 talpid-tunnel-config-client = { path = "../talpid-tunnel-config-client" }
 mullvad-encrypted-dns-proxy = { path = "../mullvad-encrypted-dns-proxy" }
diff --git a/mullvad-ios/src/api_client/api.rs b/mullvad-ios/src/api_client/api.rs
@@ -2,11 +2,13 @@ use mullvad_api::{
     rest::{self, MullvadRestHandle},
     ApiProxy,
 };
+use talpid_future::retry::retry_future;
 
 use super::{
     cancellation::{RequestCancelHandle, SwiftCancelHandle},
     completion::{CompletionCookie, SwiftCompletionHandler},
     response::SwiftMullvadApiResponse,
+    retry_strategy::{RetryStrategy, SwiftRetryStrategy},
     SwiftApiContext,
 };
 
@@ -24,6 +26,7 @@ use super::{
 pub unsafe extern "C" fn mullvad_api_get_addresses(
     api_context: SwiftApiContext,
     completion_cookie: *mut libc::c_void,
+    retry_strategy: SwiftRetryStrategy,
 ) -> SwiftCancelHandle {
     let completion_handler = SwiftCompletionHandler::new(CompletionCookie(completion_cookie));
 
@@ -33,10 +36,11 @@ pub unsafe extern "C" fn mullvad_api_get_addresses(
     };
 
     let api_context = api_context.into_rust_context();
+    let retry_strategy = unsafe { retry_strategy.into_rust() };
 
     let completion = completion_handler.clone();
     let task = tokio_handle.clone().spawn(async move {
-        match mullvad_api_get_addresses_inner(api_context.rest_handle()).await {
+        match mullvad_api_get_addresses_inner(api_context.rest_handle(), retry_strategy).await {
             Ok(response) => completion.finish(response),
             Err(err) => {
                 log::error!("{err:?}");
@@ -50,9 +54,18 @@ pub unsafe extern "C" fn mullvad_api_get_addresses(
 
 async fn mullvad_api_get_addresses_inner(
     rest_client: MullvadRestHandle,
+    retry_strategy: RetryStrategy,
 ) -> Result<SwiftMullvadApiResponse, rest::Error> {
     let api = ApiProxy::new(rest_client);
-    let response = api.get_api_addrs_response().await?;
+
+    let future_factory = || api.get_api_addrs_response();
+
+    let should_retry = |result: &Result<_, rest::Error>| match result {
+        Err(err) => err.is_network_error(),
+        Ok(_) => false,
+    };
+
+    let response = retry_future(future_factory, should_retry, retry_strategy.delays()).await?;
 
     SwiftMullvadApiResponse::with_body(response).await
 }
diff --git a/mullvad-ios/src/api_client/mod.rs b/mullvad-ios/src/api_client/mod.rs
@@ -10,6 +10,7 @@ mod api;
 mod cancellation;
 mod completion;
 mod response;
+mod retry_strategy;
 
 #[repr(C)]
 pub struct SwiftApiContext(*const ApiContext);
diff --git a/mullvad-ios/src/api_client/response.rs b/mullvad-ios/src/api_client/response.rs
diff --git a/mullvad-ios/src/api_client/retry_strategy.rs b/mullvad-ios/src/api_client/retry_strategy.rs

Original file line number	Diff line number	Diff line change
`@@ -10,7 +10,7 @@ import MullvadRustRuntime`
`10`	`10`	`import MullvadTypes`
`11`	`11`
`12`	`12`	`enum MullvadApiRequest {`
`13`		`- case getAddressList`
	`13`	`+ case getAddressList(retryStrategy: REST.RetryStrategy)`
`14`	`14`	`}`
`15`	`15`
`16`	`16`	`struct MullvadApiRequestFactory {`
`@@ -25,8 +25,12 @@ struct MullvadApiRequestFactory {`
`25`	`25`	`let rawPointer = Unmanaged.passRetained(pointerClass).toOpaque()`
`26`	`26`
`27`	`27`	`return switch request {`
`28`		`- case .getAddressList:`
`29`		`- MullvadApiCancellable(handle: mullvad_api_get_addresses(apiContext.context, rawPointer))`
	`28`	`+ case let .getAddressList(retryStrategy):`
	`29`	`+ MullvadApiCancellable(handle: mullvad_api_get_addresses(`
	`30`	`+ apiContext.context,`
	`31`	`+ rawPointer,`
	`32`	`+ retryStrategy.toRustStrategy()`
	`33`	`+ ))`
`30`	`34`	`}`
`31`	`35`	`}`
`32`	`36`	`}`
Original file line number	Diff line number	Diff line change
`@@ -45,10 +45,6 @@ public class MullvadApiResponse {`
`45`	`45`	`}`
`46`	`46`	`}`
`47`	`47`
`48`		`- public var shouldRetry: Bool {`
`49`		`- response.should_retry`
`50`		`- }`
`51`		`-`
`52`	`48`	`public var success: Bool {`
`53`	`49`	`response.success`
`54`	`50`	`}`
Original file line number	Diff line number	Diff line change
`@@ -14,6 +14,10 @@ extension Duration {`
`14`	`14`	`return timeInterval.isFinite`
`15`	`15`	`}`
`16`	`16`
	`17`	`+ public var seconds: Int64 {`
	`18`	`+ return components.seconds`
	`19`	`+ }`
	`20`	`+`
`17`	`21`	`public var timeInterval: TimeInterval {`
`18`	`22`	`return TimeInterval(components.seconds) + (TimeInterval(components.attoseconds) * 1e-18)`
`19`	`23`	`}`