Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache the transportStrategy in the UserDefaults #5262

Conversation

buggmagnet
Copy link
Contributor

@buggmagnet buggmagnet commented Oct 10, 2023

This work was done in the ios-prepare-2023.4 branch, but was never back ported to main.
This PR fixes that.


This change is Reviewable

@buggmagnet buggmagnet added the iOS Issues related to iOS label Oct 10, 2023
@buggmagnet buggmagnet self-assigned this Oct 10, 2023
@linear
Copy link

linear bot commented Oct 10, 2023

IOS-341 Reintroduce caching the TransportStrategy in UserDefaults

We made a last minute improvement for the transport strategy in 2023.4. We should merge this to main to not lose these changes. The change resides in commit 2dbf59551.

Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)


ios/MullvadTypes/AttemptsRecording.swift line 15 at r1 (raw file):

/// Used by `TransportStrategy` to record failed connection attempts in cache.
public protocol AttemptsRecording {
    func record(_ attempts: Int)

This method isn't called anywhere in this diff. Should this protocol exist?

Copy link
Contributor Author

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


ios/MullvadTypes/AttemptsRecording.swift line 15 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

This method isn't called anywhere in this diff. Should this protocol exist?

It shouldnt, I thought I had deleted that file.
Thanks for catching it !

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pronebird)

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 44 at r1 (raw file):

        let shadowsocksCache = ShadowsocksConfigurationCache(cacheDirectory: containerURL)

        // This init cannot fail as long as the security group identifier is valid

Perhaps missing:

// swiftlint:disable:next force_cast

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


ios/MullvadREST/RESTTransportStrategy.swift line 2 at r1 (raw file):

//
//  RESTTransportStrategy.swift

Mhm I think I renamed it with "REST" prefix a while ago.


ios/MullvadREST/RESTTransportStrategy.swift line 10 at r1 (raw file):

import Foundation
import MullvadTypes

Probably unused import.

@buggmagnet buggmagnet force-pushed the reintroduce-caching-the-transportstrategy-in-userdefaults-ios-341 branch from e85d151 to 2e299a0 Compare October 11, 2023 06:47
Copy link
Contributor Author

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 5 of 7 files reviewed, 4 unresolved discussions (waiting on @pinkisemils and @pronebird)


ios/MullvadREST/RESTTransportStrategy.swift line 2 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Mhm I think I renamed it with "REST" prefix a while ago.

Done


ios/MullvadREST/RESTTransportStrategy.swift line 10 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Probably unused import.

Done.

Copy link
Contributor Author

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 5 of 7 files reviewed, 4 unresolved discussions (waiting on @pinkisemils and @pronebird)


ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 44 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Perhaps missing:

// swiftlint:disable:next force_cast

Huh that's odd, I don't get that warning either in the CLI, or in Xcode...
Thanks for catching that up. The same fix needs to be applied in AppDelegate too.

Copy link
Contributor Author

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 5 of 7 files reviewed, 4 unresolved discussions (waiting on @pinkisemils and @pronebird)


ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 44 at r1 (raw file):

Previously, buggmagnet wrote…

Huh that's odd, I don't get that warning either in the CLI, or in Xcode...
Thanks for catching that up. The same fix needs to be applied in AppDelegate too.

Ok, the reason why this was not flagged up is twofold:

  • This is not a force_cast but a force_unwrapping triggering example
  • We did not enable force_unwrapping

As this code cannot possibly fail, I will leave it as is, and I opened a discussion about our use of force_unwrapping for our linter rules

Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @pronebird)

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pinkisemils)

Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 44 at r1 (raw file):

Previously, buggmagnet wrote…

Ok, the reason why this was not flagged up is twofold:

  • This is not a force_cast but a force_unwrapping triggering example
  • We did not enable force_unwrapping

As this code cannot possibly fail, I will leave it as is, and I opened a discussion about our use of force_unwrapping for our linter rules

This can be tackled separately.

@buggmagnet buggmagnet force-pushed the reintroduce-caching-the-transportstrategy-in-userdefaults-ios-341 branch from 2e299a0 to 1c524d5 Compare October 12, 2023 11:52
@buggmagnet buggmagnet merged commit 558858d into main Oct 12, 2023
4 checks passed
@buggmagnet buggmagnet deleted the reintroduce-caching-the-transportstrategy-in-userdefaults-ios-341 branch October 12, 2023 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants