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.

Sign up for GitHub

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

[feat] Concurrency improvements #52

Open
wants to merge 45 commits into
base: dev
Choose a base branch
from
Open

[feat] Concurrency improvements #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
a55d385
[feat] enable complete swift concurrency checking
matejmolnar Nov 24, 2023
06a9819
[fix] change shared static var property into get only
matejmolnar Nov 24, 2023
641bc10
[feat] add sendable conformances
matejmolnar Nov 24, 2023
ca85c82
[feat] conform authorization protocols to Actor
matejmolnar Nov 24, 2023
fbaa27b
[feat] conform RetryConfiguration to sendable
matejmolnar Nov 24, 2023
71f2a19
[chore] supress non-sendable type 'AnyCancellables' warning
matejmolnar Nov 24, 2023
196b8f7
[fix] DownloadManager swift concurrency warnings
matejmolnar Nov 24, 2023
7ba3e6b
[fix] UploadManager swift concurrency warnings
matejmolnar Nov 27, 2023
b8a20bc
[fix] warning: reference to static mutable property not thread safe
matejmolnar Nov 27, 2023
155dbb1
[feat] make processors and adapters conform to Sendable
matejmolnar Nov 27, 2023
e617286
[feat] add FileManager sendable conformance
matejmolnar Nov 27, 2023
7ca81c7
[feat] change EndpointRequestStorageProcessor and MultipeerConnectivi…
matejmolnar Nov 27, 2023
78b039f
[fix] lint warning
matejmolnar Dec 4, 2023
4107039
[fix] MultipeerConnectivityManager init crash
matejmolnar Dec 4, 2023
410f39f
[chore] remove unnecessary final keywords
matejmolnar Dec 4, 2023
bd75c8d
[fix] UploadService deinit retain cycle with singleton
matejmolnar Dec 4, 2023
82c53a3
[fix] tests
matejmolnar Dec 4, 2023
f75e1ed
[chore] revert let to lazy var
matejmolnar Dec 5, 2023
644e5e4
[chore] fix comment types
matejmolnar Dec 18, 2023
8bdffbe
[feat] change Actor protocol conformances to Sendable
matejmolnar Dec 18, 2023
9e72630
[chore] update package config
matejmolnar Dec 18, 2023
d385d34
[feat] change internal var to let
matejmolnar Dec 18, 2023
3386b03
[feat] add NetworkingActor
matejmolnar Dec 29, 2023
5bde238
[feat] conform DownloadAPIManaging to NetworkingActor
matejmolnar Dec 29, 2023
bde749f
[feat] add APIManagerTests
matejmolnar Dec 29, 2023
41ecd24
[feat] conform UploadAPIManaging to NetworkingActor
matejmolnar Dec 29, 2023
fb2fc95
[chore] remove unnecessary ThreadSafeDictionary
matejmolnar Dec 30, 2023
9febe71
[feat] conform other protocols and classes to @NetworkingActor
matejmolnar Dec 30, 2023
1effc96
[fix] concurrency warning
matejmolnar Dec 30, 2023
ab9ebcc
[chore] add comments
matejmolnar Dec 30, 2023
2312444
[feat] add multithread DownloadAPIManagerTests
matejmolnar Jan 1, 2024
f2bff45
[feat] change configuration
matejmolnar Jan 1, 2024
9dd468a
[chore] fix typo
matejmolnar Jan 1, 2024
24b7b30
[feat] add UploadAPIManagerTests
matejmolnar Jan 1, 2024
f54eb2b
Update Tests/NetworkingTests/UploadAPIManagerTests.swift
matejmolnar Jan 2, 2024
2590c04
Update Tests/NetworkingTests/DownloadAPIManagerTests.swift
matejmolnar Jan 2, 2024
3481616
[fix] multithread managers tests
matejmolnar Jan 2, 2024
c45f120
[fix] manager tests
matejmolnar Jan 3, 2024
dee8532
[chore] revert some classes back to open
matejmolnar Jan 8, 2024
9645825
[chore] update file name comments
matejmolnar Jan 9, 2024
296c372
[chore] remove unnecessary explicit Sendable conformance
matejmolnar Jan 9, 2024
987d49d
[chore] add final statement
matejmolnar Jan 9, 2024
d6613e0
[feat] change class to open
matejmolnar Jan 11, 2024
0b170b4
[feat] remove unnecessary async statement
matejmolnar Jan 11, 2024
7b8c5df
[fix] DownloadAPIManager tests by adjusting asyncResponse method
matejmolnar Jan 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions NetworkingSampleApp/NetworkingSampleApp.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,6 +553,7 @@
);
PRODUCT_BUNDLE_IDENTIFIER = "com.strv.networking-sample-app";
PRODUCT_NAME = "$(TARGET_NAME)";
SWIFT_STRICT_CONCURRENCY = complete;
SWIFT_VERSION = 5.0;
TARGETED_DEVICE_FAMILY = "1,2";
};
Expand All @@ -573,6 +574,7 @@
);
PRODUCT_BUNDLE_IDENTIFIER = "com.strv.networking-sample-app";
PRODUCT_NAME = "$(TARGET_NAME)";
SWIFT_STRICT_CONCURRENCY = complete;
SWIFT_VERSION = 5.0;
TARGETED_DEVICE_FAMILY = "1,2";
};
Expand Down
86 changes: 86 additions & 0 deletions ...p/NetworkingSampleApp.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
{
"pins" : [
{
"identity" : "collectionconcurrencykit",
"kind" : "remoteSourceControl",
"location" : "https://github.com/JohnSundell/CollectionConcurrencyKit.git",
"state" : {
"revision" : "b4f23e24b5a1bff301efc5e70871083ca029ff95",
"version" : "0.2.0"
}
},
{
"identity" : "cryptoswift",
"kind" : "remoteSourceControl",
"location" : "https://github.com/krzyzanowskim/CryptoSwift.git",
"state" : {
"revision" : "32f641cf24fc7abc1c591a2025e9f2f572648b0f",
"version" : "1.7.2"
}
},
{
"identity" : "sourcekitten",
"kind" : "remoteSourceControl",
"location" : "https://github.com/jpsim/SourceKitten.git",
"state" : {
"revision" : "b6dc09ee51dfb0c66e042d2328c017483a1a5d56",
"version" : "0.34.1"
}
},
{
"identity" : "swift-argument-parser",
"kind" : "remoteSourceControl",
"location" : "https://github.com/apple/swift-argument-parser.git",
"state" : {
"revision" : "8f4d2753f0e4778c76d5f05ad16c74f707390531",
"version" : "1.2.3"
}
},
{
"identity" : "swift-syntax",
"kind" : "remoteSourceControl",
"location" : "https://github.com/apple/swift-syntax.git",
"state" : {
"revision" : "74203046135342e4a4a627476dd6caf8b28fe11b",
"version" : "509.0.0"
}
},
{
"identity" : "swiftlint",
"kind" : "remoteSourceControl",
"location" : "https://github.com/realm/SwiftLint.git",
"state" : {
"revision" : "6d2e58271ebc14c37bf76d7c9f4082cc15bad718",
"version" : "0.53.0"
}
},
{
"identity" : "swiftytexttable",
"kind" : "remoteSourceControl",
"location" : "https://github.com/scottrhoyt/SwiftyTextTable.git",
"state" : {
"revision" : "c6df6cf533d120716bff38f8ff9885e1ce2a4ac3",
"version" : "0.9.0"
}
},
{
"identity" : "swxmlhash",
"kind" : "remoteSourceControl",
"location" : "https://github.com/drmohundro/SWXMLHash.git",
"state" : {
"revision" : "a853604c9e9a83ad9954c7e3d2a565273982471f",
"version" : "7.0.2"
}
},
{
"identity" : "yams",
"kind" : "remoteSourceControl",
"location" : "https://github.com/jpsim/Yams.git",
"state" : {
"revision" : "0d9ee7ea8c4ebd4a489ad7a73d5c6cad55d6fed3",
"version" : "5.0.6"
}
}
],
"version" : 2
}
1 change: 1 addition & 0 deletions NetworkingSampleApp/NetworkingSampleApp/API/SampleAuthorizationManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import Networking
import Foundation

@NetworkingActor
final class SampleAuthorizationManager: AuthorizationManaging {
// MARK: Public properties
let storage: AuthorizationStorageManaging = SampleAuthorizationStorageManager()
Expand Down
2 changes: 1 addition & 1 deletion NetworkingSampleApp/NetworkingSampleApp/API/SampleAuthorizationStorageManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import Networking

actor SampleAuthorizationStorageManager: AuthorizationStorageManaging {
final class SampleAuthorizationStorageManager: AuthorizationStorageManaging {
private var storage: AuthorizationData?

func saveData(_ data: AuthorizationData) async throws {
Expand Down
4 changes: 2 additions & 2 deletions NetworkingSampleApp/NetworkingSampleApp/API/SampleErrorProcessor.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ import Foundation

/// Maps NetworkError's unacceptableStatusCode 400 error to SampleAPIError.
final class SampleErrorProcessor: ErrorProcessing {
private lazy var decoder = JSONDecoder()
private let decoder = JSONDecoder()

func process(_ error: Error, for endpointRequest: EndpointRequest) -> Error {
guard let networkError = error as? NetworkError,
case let .unacceptableStatusCode(statusCode, _, response) = networkError,
Expand Down
2 changes: 1 addition & 1 deletion NetworkingSampleApp/NetworkingSampleApp/Extensions/DownloadAPIManager+SharedInstance.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import Networking

extension DownloadAPIManager {
static var shared: DownloadAPIManaging = {
static let shared: DownloadAPIManager = {
var responseProcessors: [ResponseProcessing] = [
LoggingInterceptor.shared,
StatusCodeProcessor.shared
Expand Down
2 changes: 2 additions & 0 deletions NetworkingSampleApp/NetworkingSampleApp/Scenes/Authorization/AuthorizationViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ import Foundation
import Networking

final class AuthorizationViewModel: ObservableObject {
@NetworkingActor
private lazy var authManager = SampleAuthorizationManager()
@NetworkingActor
private lazy var apiManager: APIManager = {
let authorizationInterceptor = AuthorizationTokenInterceptor(authorizationManager: authManager)

Expand Down
2 changes: 1 addition & 1 deletion NetworkingSampleApp/NetworkingSampleApp/Scenes/Download/DownloadProgressViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ final class DownloadProgressViewModel: ObservableObject {
}

func startObservingDownloadProgress() async {
let stream = DownloadAPIManager.shared.progressStream(for: task)
let stream = await DownloadAPIManager.shared.progressStream(for: task)

for try await downloadState in stream {
var newState = DownloadProgressState()
Expand Down
8 changes: 2 additions & 6 deletions NetworkingSampleApp/NetworkingSampleApp/Scenes/Upload/FormUploadsViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,7 @@ final class FormUploadsViewModel: ObservableObject {
return fileName
}

private let uploadService: UploadService

init(uploadService: UploadService = .init()) {
self.uploadService = uploadService
}
private let uploadService = UploadService.shared
Copy link
Collaborator

Choose a reason for hiding this comment

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

why did we remove init method? I commented on UploadsViewModel the different approach we used

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The service will be removed in #59

}

extension FormUploadsViewModel {
Expand Down Expand Up @@ -58,7 +54,7 @@ extension FormUploadsViewModel {
// MARK: - Prepare multipartForm data
private extension FormUploadsViewModel {
func createMultipartFormData() throws -> MultipartFormData {
let multipartFormData = MultipartFormData()
var multipartFormData = MultipartFormData()
multipartFormData.append(Data(username.utf8), name: "username-textfield")
if let fileUrl {
try multipartFormData.append(from: fileUrl, name: "attachment")
Expand Down
2 changes: 2 additions & 0 deletions NetworkingSampleApp/NetworkingSampleApp/Scenes/Upload/UploadItemViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
//

import Foundation
// This import suppresses warning: Non-sendable type 'AsyncPublisher<AnyPublisher<UploadTask.State, Never>>' ...
@preconcurrency import Combine

@MainActor
final class UploadItemViewModel: ObservableObject {
Expand Down
15 changes: 5 additions & 10 deletions NetworkingSampleApp/NetworkingSampleApp/Scenes/Upload/UploadService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,11 @@
import Foundation
import Networking

final class UploadService {
private let uploadManager: UploadAPIManaging
@NetworkingActor
final class UploadService: Sendable {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why also service needs to be sendable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The service will be removed in #59

static let shared = UploadService()

init(uploadManager: UploadAPIManaging = UploadAPIManager()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we resign on injection approach here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The service will be removed in #59

self.uploadManager = uploadManager
}

deinit {
uploadManager.invalidateSession(shouldFinishTasks: false)
}
johnkodes marked this conversation as resolved.
Show resolved Hide resolved
private let uploadManager = UploadAPIManager()
}

extension UploadService {
Expand Down Expand Up @@ -60,7 +55,7 @@ extension UploadService {
}

func uploadStateStream(for uploadTaskId: String) async -> UploadAPIManaging.StateStream {
await uploadManager.stateStream(for: uploadTaskId)
uploadManager.stateStream(for: uploadTaskId)
}

func pause(taskId: String) async {
Expand Down
2 changes: 1 addition & 1 deletion NetworkingSampleApp/NetworkingSampleApp/Scenes/Upload/UploadsViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ final class UploadsViewModel: ObservableObject {

private let uploadService: UploadService

init(uploadService: UploadService = UploadService()) {
init(uploadService: UploadService = .shared) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a little bit confused. In FormUploadsViewModel we remove init with UploadService injection and we have just let variable with shared instance. I would prefer to have everywhere option to inject the service at least for the showcase, no matter we use just shared instance for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The service will be removed in #59

self.uploadService = uploadService
}
}
Expand Down
9 changes: 8 additions & 1 deletion NetworkingSampleApp/NetworkingSampleApp/Scenes/Users/UsersViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ final class UsersViewModel: ObservableObject {
return decoder
}()

@NetworkingActor
private lazy var apiManager: APIManager = {
var responseProcessors: [ResponseProcessing] = [
LoggingInterceptor.shared,
Expand Down Expand Up @@ -57,7 +58,13 @@ extension UsersViewModel {
}
}

return try await group.reduce(into: [User]()) { $0.append($1) }
var results = [User]()
Copy link
Collaborator

Choose a reason for hiding this comment

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

why did we rewrite this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was getting the warning mentioned in this thread, however there is no response how to handle it, hence I rather decided to adjust the implementation 🤷‍♂️


for try await value in group {
results.append(value)
}

return results
}
} else {
// Fetch user add it to users array and wait for 0.5 seconds, before fetching the next one.
Expand Down
86 changes: 86 additions & 0 deletions Package.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
{
"pins" : [
{
"identity" : "collectionconcurrencykit",
"kind" : "remoteSourceControl",
"location" : "https://github.com/JohnSundell/CollectionConcurrencyKit.git",
"state" : {
"revision" : "b4f23e24b5a1bff301efc5e70871083ca029ff95",
"version" : "0.2.0"
}
},
{
"identity" : "cryptoswift",
"kind" : "remoteSourceControl",
"location" : "https://github.com/krzyzanowskim/CryptoSwift.git",
"state" : {
"revision" : "32f641cf24fc7abc1c591a2025e9f2f572648b0f",
"version" : "1.7.2"
}
},
{
"identity" : "sourcekitten",
"kind" : "remoteSourceControl",
"location" : "https://github.com/jpsim/SourceKitten.git",
"state" : {
"revision" : "b6dc09ee51dfb0c66e042d2328c017483a1a5d56",
"version" : "0.34.1"
}
},
{
"identity" : "swift-argument-parser",
"kind" : "remoteSourceControl",
"location" : "https://github.com/apple/swift-argument-parser.git",
"state" : {
"revision" : "8f4d2753f0e4778c76d5f05ad16c74f707390531",
"version" : "1.2.3"
}
},
{
"identity" : "swift-syntax",
"kind" : "remoteSourceControl",
"location" : "https://github.com/apple/swift-syntax.git",
"state" : {
"revision" : "74203046135342e4a4a627476dd6caf8b28fe11b",
"version" : "509.0.0"
}
},
{
"identity" : "swiftlint",
"kind" : "remoteSourceControl",
"location" : "https://github.com/realm/SwiftLint.git",
"state" : {
"revision" : "6d2e58271ebc14c37bf76d7c9f4082cc15bad718",
"version" : "0.53.0"
}
},
{
"identity" : "swiftytexttable",
"kind" : "remoteSourceControl",
"location" : "https://github.com/scottrhoyt/SwiftyTextTable.git",
"state" : {
"revision" : "c6df6cf533d120716bff38f8ff9885e1ce2a4ac3",
"version" : "0.9.0"
}
},
{
"identity" : "swxmlhash",
"kind" : "remoteSourceControl",
"location" : "https://github.com/drmohundro/SWXMLHash.git",
"state" : {
"revision" : "a853604c9e9a83ad9954c7e3d2a565273982471f",
"version" : "7.0.2"
}
},
{
"identity" : "yams",
"kind" : "remoteSourceControl",
"location" : "https://github.com/jpsim/Yams.git",
"state" : {
"revision" : "0d9ee7ea8c4ebd4a489ad7a73d5c6cad55d6fed3",
"version" : "5.0.6"
}
}
],
"version" : 2
}
1 change: 1 addition & 0 deletions Package.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ let package = Package(
// Targets can depend on other targets in this package, and on products in packages this package depends on.
.target(
name: "Networking",
swiftSettings: [.enableExperimentalFeature("StrictConcurrency")],
plugins: [.plugin(name: "SwiftLintPlugin", package: "SwiftLint")]
),
.testTarget(
Expand Down
6 changes: 3 additions & 3 deletions Sources/Networking/Core/APIManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ open class APIManager: APIManaging, Retryable {
private let errorProcessors: [ErrorProcessing]
private let responseProvider: ResponseProviding
private let sessionId: String
internal var retryCounter = Counter()
let retryCounter = Counter()

public init(
urlSession: URLSession = .init(configuration: .default),
requestAdapters: [RequestAdapting] = [],
Expand Down Expand Up @@ -69,7 +69,7 @@ private extension APIManager {
response = try await responseProcessors.process(response, with: request, for: endpointRequest)

/// reset retry count
await retryCounter.reset(for: endpointRequest.id)
retryCounter.reset(for: endpointRequest.id)

return response
} catch {
Expand Down
3 changes: 2 additions & 1 deletion Sources/Networking/Core/APIManaging.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ import Foundation
// MARK: - Defines API managing

/// A definition of an API layer with methods for handling API requests.
public protocol APIManaging {
@NetworkingActor
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we explain how does this work? When I define global actor for protocol, does it mean that all protocol methods by default run in this actor?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here is a good explanation https://www.hackingwithswift.com/quick-start/concurrency/understanding-how-global-actor-inference-works

public protocol APIManaging: Sendable {
/// A default JSONDecoder used for all requests.
var defaultDecoder: JSONDecoder { get }

Expand Down
Loading