From f2bee4d5a33fa896356ff8175407bef1d4c62e78 Mon Sep 17 00:00:00 2001 From: mojganii Date: Tue, 23 Jan 2024 14:32:54 +0100 Subject: [PATCH] reusing existing connection for upcoming requests if it is still valid --- .../Transport/AccessMethodIterator.swift | 19 ++++---- .../ShadowsocksConfiguration.swift | 2 +- .../Socks5/Socks5Configuration.swift | 2 +- .../Transport/TransportProvider.swift | 46 ++++++++++++------- .../Transport/TransportStrategy.swift | 15 +++++- .../AccessMethodRepositoryStub.swift | 5 +- .../TransportStrategyTests.swift | 17 ------- .../AccessMethodRepositoryProtocol.swift | 9 ++-- 8 files changed, 65 insertions(+), 50 deletions(-) diff --git a/ios/MullvadREST/Transport/AccessMethodIterator.swift b/ios/MullvadREST/Transport/AccessMethodIterator.swift index a91c0fd54633..34ec668c6404 100644 --- a/ios/MullvadREST/Transport/AccessMethodIterator.swift +++ b/ios/MullvadREST/Transport/AccessMethodIterator.swift @@ -15,9 +15,12 @@ class AccessMethodIterator { private let dataSource: AccessMethodRepositoryDataSource private var index = 0 - private var enabledConfigurations: [PersistentAccessMethod] = [] private var cancellables = Set() + private var enabledConfigurations: [PersistentAccessMethod] { + dataSource.fetchAll().filter { $0.isEnabled } + } + private var lastReachableApiAccessId: UUID { lastReachableApiAccessCache.id } @@ -31,9 +34,8 @@ class AccessMethodIterator { self.dataSource .publisher - .sink { [weak self] configurations in + .sink { [weak self] _ in guard let self else { return } - self.enabledConfigurations = configurations.filter { $0.isEnabled } self.refreshCacheIfNeeded() } .store(in: &cancellables) @@ -45,7 +47,7 @@ class AccessMethodIterator { index = firstIndex } else { /// When `firstIndex` is `nil`, that means the current configuration is not valid anymore - /// Invalidating cache by replacing the `current` to the next enabled access method + /// Invalidating cache by replacing the `current` to the next enabled access method lastReachableApiAccessCache.id = pick().id } } @@ -57,14 +59,15 @@ class AccessMethodIterator { } func pick() -> PersistentAccessMethod { - if enabledConfigurations.isEmpty { - /// Returning `Default` strategy when all is disabled + let configurations = enabledConfigurations + if configurations.isEmpty { + /// Returning `Default` strategy when all is disabled return dataSource.directAccess } else { /// Picking the next `Enabled` configuration in order they are added /// And starting from the beginning when it reaches end - let circularIndex = index % enabledConfigurations.count - return enabledConfigurations[circularIndex] + let circularIndex = index % configurations.count + return configurations[circularIndex] } } } diff --git a/ios/MullvadREST/Transport/Shadowsocks/ShadowsocksConfiguration.swift b/ios/MullvadREST/Transport/Shadowsocks/ShadowsocksConfiguration.swift index 710b3ef06b82..4c9b1a7d6bd2 100644 --- a/ios/MullvadREST/Transport/Shadowsocks/ShadowsocksConfiguration.swift +++ b/ios/MullvadREST/Transport/Shadowsocks/ShadowsocksConfiguration.swift @@ -10,7 +10,7 @@ import Foundation import MullvadTypes import Network -public struct ShadowsocksConfiguration: Codable { +public struct ShadowsocksConfiguration: Codable, Equatable { public let address: AnyIPAddress public let port: UInt16 public let password: String diff --git a/ios/MullvadREST/Transport/Socks5/Socks5Configuration.swift b/ios/MullvadREST/Transport/Socks5/Socks5Configuration.swift index 4f7ebc06f94b..9bf2eb87dc2b 100644 --- a/ios/MullvadREST/Transport/Socks5/Socks5Configuration.swift +++ b/ios/MullvadREST/Transport/Socks5/Socks5Configuration.swift @@ -11,7 +11,7 @@ import MullvadTypes /// Socks5 configuration. /// - See: ``URLSessionSocks5Transport`` -public struct Socks5Configuration { +public struct Socks5Configuration: Equatable { /// The socks proxy endpoint. public var proxyEndpoint: AnyIPEndpoint diff --git a/ios/MullvadREST/Transport/TransportProvider.swift b/ios/MullvadREST/Transport/TransportProvider.swift index 10d2feafeb52..06aaf6395ce1 100644 --- a/ios/MullvadREST/Transport/TransportProvider.swift +++ b/ios/MullvadREST/Transport/TransportProvider.swift @@ -15,6 +15,7 @@ public final class TransportProvider: RESTTransportProvider { private let addressCache: REST.AddressCache private var transportStrategy: TransportStrategy private var currentTransport: RESTTransport? + private var currentTransportType: TransportStrategy.Transport private let parallelRequestsMutex = NSLock() public init( @@ -25,6 +26,7 @@ public final class TransportProvider: RESTTransportProvider { self.urlSessionTransport = urlSessionTransport self.addressCache = addressCache self.transportStrategy = transportStrategy + self.currentTransportType = transportStrategy.connectionTransport() } public func makeTransport() -> RESTTransport? { @@ -62,26 +64,36 @@ public final class TransportProvider: RESTTransportProvider { /// /// - Returns: A `RESTTransport` object to make a connection private func makeTransportInner() -> RESTTransport? { - switch transportStrategy.connectionTransport() { - case .direct: - currentTransport = urlSessionTransport - case let .shadowsocks(configuration): - currentTransport = ShadowsocksTransport( - urlSession: urlSessionTransport.urlSession, - configuration: configuration, - addressCache: addressCache - ) - case let .socks5(configuration): - currentTransport = URLSessionSocks5Transport( - urlSession: urlSessionTransport.urlSession, - configuration: configuration, - addressCache: addressCache - ) - case .none: - currentTransport = nil + if currentTransport == nil || shouldNotReuseCurrentTransport { + currentTransportType = transportStrategy.connectionTransport() + switch currentTransportType { + case .direct: + currentTransport = urlSessionTransport + case let .shadowsocks(configuration): + currentTransport = ShadowsocksTransport( + urlSession: urlSessionTransport.urlSession, + configuration: configuration, + addressCache: addressCache + ) + case let .socks5(configuration): + currentTransport = URLSessionSocks5Transport( + urlSession: urlSessionTransport.urlSession, + configuration: configuration, + addressCache: addressCache + ) + case .none: + currentTransport = nil + } } return currentTransport } + + /// The `Main` allows modifications to access methods through the UI. + /// The `TransportProvider` relies on a `CurrentTransport` value set during build time or network error. + /// To ensure both process `Packet Tunnel` and `Main` uses the latest changes, the `TransportProvider` compares the `transportType` with the latest value in the cache and reuse it if it's still valid . + private var shouldNotReuseCurrentTransport: Bool { + currentTransportType != transportStrategy.connectionTransport() + } } private extension URLError { diff --git a/ios/MullvadREST/Transport/TransportStrategy.swift b/ios/MullvadREST/Transport/TransportStrategy.swift index a63891c601b0..acb132acabd3 100644 --- a/ios/MullvadREST/Transport/TransportStrategy.swift +++ b/ios/MullvadREST/Transport/TransportStrategy.swift @@ -13,7 +13,7 @@ import MullvadTypes public class TransportStrategy: Equatable { /// The different transports suggested by the strategy - public enum Transport { + public enum Transport: Equatable { /// Connecting a direct connection case direct @@ -25,6 +25,19 @@ public class TransportStrategy: Equatable { /// Failing to retrive transport case none + + public static func == (lhs: Self, rhs: Self) -> Bool { + switch (lhs, rhs) { + case(.direct, .direct), (.none, .none): + return true + case let (.shadowsocks(lhsConfiguration), .shadowsocks(rhsConfiguration)): + return lhsConfiguration == rhsConfiguration + case let (.socks5(lhsConfiguration), .socks5(rhsConfiguration)): + return lhsConfiguration == rhsConfiguration + default: + return false + } + } } private let shadowsocksLoader: ShadowsocksLoaderProtocol diff --git a/ios/MullvadRESTTests/AccessMethodRepositoryStub.swift b/ios/MullvadRESTTests/AccessMethodRepositoryStub.swift index accd7e0f7ce6..bd8b34165422 100644 --- a/ios/MullvadRESTTests/AccessMethodRepositoryStub.swift +++ b/ios/MullvadRESTTests/AccessMethodRepositoryStub.swift @@ -12,7 +12,6 @@ import MullvadSettings typealias PersistentAccessMethod = MullvadSettings.PersistentAccessMethod struct AccessMethodRepositoryStub: AccessMethodRepositoryDataSource { var directAccess: MullvadSettings.PersistentAccessMethod - var publisher: AnyPublisher<[MullvadSettings.PersistentAccessMethod], Never> { passthroughSubject.eraseToAnyPublisher() } @@ -23,4 +22,8 @@ struct AccessMethodRepositoryStub: AccessMethodRepositoryDataSource { directAccess = accessMethods.first(where: { $0.kind == .direct })! passthroughSubject.send(accessMethods) } + + func fetchAll() -> [MullvadSettings.PersistentAccessMethod] { + passthroughSubject.value + } } diff --git a/ios/MullvadRESTTests/TransportStrategyTests.swift b/ios/MullvadRESTTests/TransportStrategyTests.swift index eee045fec28e..9acc9e443743 100644 --- a/ios/MullvadRESTTests/TransportStrategyTests.swift +++ b/ios/MullvadRESTTests/TransportStrategyTests.swift @@ -238,23 +238,6 @@ class TransportStrategyTests: XCTestCase { } } -extension TransportStrategy.Transport: Equatable { - public static func == (lhs: Self, rhs: Self) -> Bool { - switch (lhs, rhs) { - case(.direct, .direct), (.none, .none): - return true - case let (.shadowsocks(config1), .shadowsocks(config2)): - return config1.port == config2.port && config1.cipher == config2.cipher && config1.password == config2 - .password - case let (.socks5(config1), .socks5(config2)): - return config1.proxyEndpoint == config2.proxyEndpoint && config1.username == config2.username && config1 - .password == config2.password - default: - return false - } - } -} - private enum IOError: Error { case fileNotFound } diff --git a/ios/MullvadSettings/AccessMethodRepositoryProtocol.swift b/ios/MullvadSettings/AccessMethodRepositoryProtocol.swift index f956b37848d6..9b0a255b9984 100644 --- a/ios/MullvadSettings/AccessMethodRepositoryProtocol.swift +++ b/ios/MullvadSettings/AccessMethodRepositoryProtocol.swift @@ -13,7 +13,12 @@ public protocol AccessMethodRepositoryDataSource { /// Publisher that propagates a snapshot of persistent store upon modifications. var publisher: AnyPublisher<[PersistentAccessMethod], Never> { get } + /// - Returns: the default strategy. var directAccess: PersistentAccessMethod { get } + + /// Fetch all access method from the persistent store. + /// - Returns: an array of all persistent access method. + func fetchAll() -> [PersistentAccessMethod] } public protocol AccessMethodRepositoryProtocol: AccessMethodRepositoryDataSource { @@ -34,10 +39,6 @@ public protocol AccessMethodRepositoryProtocol: AccessMethodRepositoryDataSource /// - Returns: a persistent access method model upon success, otherwise `nil`. func fetch(by id: UUID) -> PersistentAccessMethod? - /// Fetch all access method from the persistent store. - /// - Returns: an array of all persistent access method. - func fetchAll() -> [PersistentAccessMethod] - /// Refreshes the storage with default values. func reloadWithDefaultsAfterDataRemoval() }