reusing existing connection for requests

[mojganii]

This pull request optimizes performance by enabling the reuse of the current connection, eliminating the necessity for a new connection with each incoming request. However, exceptions are made in cases such as request failures or modifications to the active configuration by other processes (e.g., Packet tunnel, UI part).

---

This change is 

[linear]

IOS-449 Reuse existing connection for requests

[rablador]

Reviewed 8 of 9 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mojganii)

---

ios/MullvadSettings/AccessMethodRepository.swift line 35 at r2 (raw file):

    }

    public var accessMethods: [PersistentAccessMethod] {

Can't we use fetchAll directly instead since it's already public.

[mojganii]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rablador)

---

ios/MullvadSettings/AccessMethodRepository.swift line 35 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

Can't we use fetchAll directly instead since it's already public.

Done.

[rablador]

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[buggmagnet]

Reviewed 5 of 9 files at r1, 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mojganii)

---

ios/MullvadREST/Transport/AccessMethodIterator.swift line 13 at r3 (raw file):

import MullvadSettings

class AccessMethodIterator {

nit
It just occured to me that this is named Iterator but doesn't implement the Iterator protocol, which is a misleading name.
Not a big deal, but we should be better at naming things in the future

---

ios/MullvadREST/Transport/AccessMethodIterator.swift line 50 at r3 (raw file):

        } 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

nit
There 2 spaces in current to

---

ios/MullvadREST/Transport/AccessMethodIterator.swift line 62 at r3 (raw file):

    func pick() -> PersistentAccessMethod {
        if enabledConfigurations.isEmpty {

Given that enabledConfigurations is really

dataSource.fetchAll().filter { $0.isEnabled }

we're calling this at best once, at worst 3 times every time we call pick(), which would be really bad if dataSource is a huge array to filter.

Suggestion

    func pick() -> PersistentAccessMethod {
        let configurations = enabledConfigurations
        if configurations.isEmpty {
            /// Returning `directAccess` when all methods are 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 % configurations.count
            return configurations[circularIndex]
        }
    }

---

ios/MullvadREST/Transport/AccessMethodIterator.swift line 63 at r3 (raw file):

    func pick() -> PersistentAccessMethod {
        if enabledConfigurations.isEmpty {
            /// Returning  `Default` strategy  when  all is disabled

nit
there are 2 spaces in Returning Default

[mojganii]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadREST/Transport/AccessMethodIterator.swift line 13 at r3 (raw file):

Previously, buggmagnet wrote…

nit
It just occured to me that this is named Iterator but doesn't implement the Iterator protocol, which is a misleading name.
Not a big deal, but we should be better at naming things in the future

do you have any suggestion? how about AccessMethodRotator?

---

ios/MullvadREST/Transport/AccessMethodIterator.swift line 50 at r3 (raw file):

Previously, buggmagnet wrote…

nit
There 2 spaces in current to

Done

---

ios/MullvadREST/Transport/AccessMethodIterator.swift line 62 at r3 (raw file):

Previously, buggmagnet wrote…

Given that enabledConfigurations is really

dataSource.fetchAll().filter { $0.isEnabled }

we're calling this at best once, at worst 3 times every time we call pick(), which would be really bad if dataSource is a huge array to filter.

Suggestion

    func pick() -> PersistentAccessMethod {
        let configurations = enabledConfigurations
        if configurations.isEmpty {
            /// Returning `directAccess` when all methods are 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 % configurations.count
            return configurations[circularIndex]
        }
    }

Done.

---

ios/MullvadREST/Transport/AccessMethodIterator.swift line 63 at r3 (raw file):

Previously, buggmagnet wrote…

nit
there are 2 spaces in Returning Default

Done

[buggmagnet]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions