[feat] Concurrency improvements #52

matejmolnar · 2023-11-27T17:45:33Z

This PR aims to solve this issue #50

Changes:

Set the strict swift concurrency build setting to complete for both the ios-networking package and for the NetworkingSampleApp.
Create a global actor @NetworkingActor where all networking related operations should synchronize.
Conform a bunch of protocols/classes to @NetworkingActor
Conform a bunch of types to Sendable
Remove ThreadSafeDictionary and replace with regular dictionaries since we are processing all operations on @NetworkingActor.
Some warnings regarding non-sendable types being used within isolated context had to be silenced by adding @preconcurrency keyword before import of a given module causing the issues (mainly Combine), with a comment explaining why it had to be done.
The biggest changes with which I've struggled the most are in MultipeerConnectivityManager and EndpointRequestStorageProcessor, mainly due to the fact that we want to get a device name by calling UIDevice.current.name which from now on has to be done on MainActor. As you can see in the code it's quite tricky to inject a MainActor parameter into another class/actor that is not a MainActor, but it can be done, even though it's not elegant at all.

Sources/Networking/Core/DownloadAPIManaging.swift

Sources/Networking/Core/RetryConfiguration.swift

johnkodes

@matejmolnar thank you for the changes 🙏

Before I'll finish the whole code review, I would like to hear some reasoning behind the changes.

I'm by no means an expert on Complete Concurrency so take the comments also as me trying to get more context.

My comments were mainly driven by how can we keep the library customizable (eg. inheritable) but thread-safe at the same time.

I wonder if we should also think about creating a global actor that will be used in the library to make the actor/thread synchronization easier.

Also, could we also maybe try adding some tests that verify thread-safety? ☺️

Sources/Networking/Modifiers/Interceptors/Authorization/AuthorizationStorageManaging.swift

Sources/Networking/Modifiers/Interceptors/LoggingInterceptor.swift

...g/Modifiers/Processors/EndpointRequestStorageProcessor/EndpointRequestStorageProcessor.swift

johnkodes · 2023-12-17T10:04:31Z

NetworkingSampleApp/NetworkingSampleApp/API/SampleAuthorizationManager.swift

@@ -8,7 +8,7 @@
 import Networking
 import Foundation

-final class SampleAuthorizationManager: AuthorizationManaging {
+actor SampleAuthorizationManager: AuthorizationManaging {


What's the different of using this final class SampleAuthorizationManager: AuthorizationManaging, Sendable versus using an actor? 🤔

This warning Stored property 'apiManager' of 'Sendable'-conforming class 'SampleAuthorizationManager' has non-sendable type 'APIManager'

Yes, this change would count with making the APIManager unchecked Sendable. Is there anything that's stopping from doing that? Everything is a let and then we have the actor for the counter. Or am I missing something? 😄

But I would love us to first write unit tests that will check thread safety of the APIManager, DownloadAPIManager etc., used as an actor and then verify that we actually need it and if not, we would safely mark it as unchecked Sendable?

NetworkingSampleApp/NetworkingSampleApp/Scenes/Upload/UploadService.swift

Sources/Networking/Core/DownloadAPIManager.swift

Sources/Networking/Core/Upload/MultipartFormDataEncoder.swift

NetworkingSampleApp/NetworkingSampleApp/Scenes/Upload/UploadService.swift

Sources/Networking/Core/DownloadAPIManager.swift

Sources/Networking/Modifiers/Interceptors/Authorization/AuthorizationManaging.swift

- Fix build error nonisolated MainActor due to UIDevice usage

…tyManager to actors

matejmolnar · 2023-12-30T16:31:15Z

@johnkodes Ok I think I've finally found the proper solution to basically all the issues we were discussing 🤞😅. I've created a single global actor @NetworkingActor and conformed all the necessary protocols/classes to it. What do you think?

Tests/NetworkingTests/APIManagerTests.swift

Tests/NetworkingTests/DownloadAPIManagerTests.swift

Tests/NetworkingTests/UploadAPIManagerTests.swift

Co-authored-by: Jan Kodeš <jan.kodes@strv.com>

johnkodes · 2024-01-02T16:23:32Z

@johnkodes Ok I think I've finally found the proper solution to basically all the issues we were discussing 🤞😅. I've created a single global actor @NetworkingActor and conformed all the necessary protocols/classes to it. What do you think?

@matejmolnar did you find some information on whether using a single Global Actor for everything could lead to performance drawbacks? 🤔 Eg. by marking everything NetworkingActor we will have everything act as a serial queue right?

matejmolnar · 2024-01-04T14:31:05Z

@johnkodes Good point. I tried researching this topic and to be honest I am a little confused. I think it should be ok, but I am not 100% sure.
regarding this statement by marking everything NetworkingActor we will have everything act as a serial queue . Are you are worried that running parallel requests won't work? Because that's not the case. You can even try the parallel requests in the users sample. You can see in the logs that the requests fire at once and after a while the responses arrive in a random order.

cejanen

Nice, comparing to the first attempt I see big progress. Most of changes are logic for me. I tried to find places and noted with questions to confirm my thoughts and eventually we should explain those places in code as well.
There are some minor issues I found and maybe for update like this we should cover library with more multithreading tests.
Some changes should forward us to API design changes itself not just to pass concurrency check without warnings.

Sources/Networking/Core/NetworkingActor.swift

cejanen · 2024-01-07T16:58:47Z

Sources/Networking/Core/NetworkingActor.swift

+import Foundation
+
+/// The only singleton actor in this library where all networking related operations should synchronize.
+@globalActor public actor NetworkingActor {


Can we have it also final? @globalActor final public actor NetworkingActor? Or are there any downsides?

The final is not really necessary since actors are non-inheritable, but there is no downside to include it if you want.

NetworkingSampleApp/NetworkingSampleApp/API/SampleAuthorizationManager.swift

cejanen · 2024-01-07T17:24:45Z

NetworkingSampleApp/NetworkingSampleApp/Scenes/Upload/UploadsViewModel.swift

@@ -16,7 +16,7 @@ final class UploadsViewModel: ObservableObject {

    private let uploadService: UploadService

-    init(uploadService: UploadService = UploadService()) {
+    init(uploadService: UploadService = .shared) {


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.

The service will be removed in #59

cejanen · 2024-01-07T17:25:34Z

NetworkingSampleApp/NetworkingSampleApp/Scenes/Upload/FormUploadsViewModel.swift

-    init(uploadService: UploadService = .init()) {
-        self.uploadService = uploadService
-    }
+    private let uploadService = UploadService.shared


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

The service will be removed in #59

Sources/Networking/Modifiers/Processors/StatusCodeProcessor.swift

Sources/Networking/Core/Upload/UploadAPIManager.swift

cejanen · 2024-01-08T12:39:32Z

Sources/Networking/Core/Upload/UploadAPIManaging.swift

@@ -63,13 +64,13 @@ public protocol UploadAPIManaging {
    /// i.e., `UploadTask.State.error` is non-nil. In such case, you can call `retry(taskId:)` to re-activate the stream for the specified `uploadTaskId`.
    /// - Parameter uploadTaskId: The identifier of the task to observe.
    /// - Returns: An asynchronous stream of upload state. If there is no such upload task the return stream finishes immediately.
-    func stateStream(for uploadTaskId: UploadTask.ID) async -> StateStream
+    func stateStream(for uploadTaskId: UploadTask.ID) -> StateStream


Similar question like for other class. Why do we change async to sync method and vice versa? Because of context where does it run?

I think it makes sense for methods that don't necessarily need to be async to let them be sync and this one fulfils that criteria. Don't you agree?

...g/Modifiers/Processors/EndpointRequestStorageProcessor/EndpointRequestStorageProcessor.swift

cejanen · 2024-01-08T12:42:35Z

...g/Modifiers/Processors/EndpointRequestStorageProcessor/EndpointRequestStorageProcessor.swift

+
+    // This would ideally also be a lazy var, however it has to be async, because UIDevice.current.name needs to be called on MainActor.
+    private var _multipeerConnectivityManager: MultipeerConnectivityManager?
+    private var multipeerConnectivityManager: MultipeerConnectivityManager? {


in case like this, we touch device just for name. I'm thinking maybe the whole design of networking is bad in this case. For this variable maybe we should inject name to the processor and we don't have to figure out where is it from (app will send it - either device name or custom value)

I know, It's a weird implementation, but otherwise it won't be possible to have a shared instance, because the static let shared cannot execute the MainActor isolated code UIDevice.current.name.

johnkodes · 2024-01-15T09:03:45Z

@ipek did you try to break this as we discussed? 😄

ipek · 2024-01-15T09:05:08Z

@ipek did you try to break this as we discussed? 😄

Soon. Lately I feel like I am juggling several contexts and have 0 work done 😅 .

matejmolnar changed the title ~~Feat/concurrency improvements~~ [WIP] Feat/concurrency improvements Nov 27, 2023

matejmolnar requested a review from a team December 4, 2023 18:29

matejmolnar changed the title ~~[WIP] Feat/concurrency improvements~~ [feat] Concurrency improvements Dec 4, 2023

matejmolnar linked an issue Dec 6, 2023 that may be closed by this pull request

SWIFT_STRICT_CONCURRENCY compiler check to complete #50

Open

johnkodes reviewed Dec 15, 2023

View reviewed changes

Sources/Networking/Core/DownloadAPIManaging.swift Outdated Show resolved Hide resolved

johnkodes reviewed Dec 15, 2023

View reviewed changes

Sources/Networking/Core/RetryConfiguration.swift Outdated Show resolved Hide resolved

johnkodes reviewed Dec 17, 2023

View reviewed changes

matejmolnar requested a review from johnkodes December 18, 2023 17:20

matejmolnar added 22 commits December 29, 2023 12:45

[feat] enable complete swift concurrency checking

a55d385

- Fix build error nonisolated MainActor due to UIDevice usage

[fix] change shared static var property into get only

06a9819

[feat] add sendable conformances

641bc10

[feat] conform authorization protocols to Actor

ca85c82

[feat] conform RetryConfiguration to sendable

fbaa27b

[chore] supress non-sendable type 'AnyCancellables' warning

71f2a19

[fix] DownloadManager swift concurrency warnings

196b8f7

[fix] UploadManager swift concurrency warnings

7ba3e6b

[fix] warning: reference to static mutable property not thread safe

b8a20bc

[feat] make processors and adapters conform to Sendable

155dbb1

[feat] add FileManager sendable conformance

e617286

[feat] change EndpointRequestStorageProcessor and MultipeerConnectivi…

7ca81c7

…tyManager to actors

[fix] lint warning

78b039f

[fix] MultipeerConnectivityManager init crash

4107039

[chore] remove unnecessary final keywords

410f39f

[fix] UploadService deinit retain cycle with singleton

bd75c8d

[fix] tests

82c53a3

[chore] revert let to lazy var

f75e1ed

[chore] fix comment types

644e5e4

[feat] change Actor protocol conformances to Sendable

8bdffbe

[chore] update package config

9e72630

[feat] change internal var to let

d385d34

matejmolnar added 6 commits December 29, 2023 15:05

[feat] add APIManagerTests

bde749f

[feat] conform UploadAPIManaging to NetworkingActor

41ecd24

[chore] remove unnecessary ThreadSafeDictionary

fb2fc95

[feat] conform other protocols and classes to @NetworkingActor

9febe71

[fix] concurrency warning

1effc96

[chore] add comments

ab9ebcc

johnkodes reviewed Jan 1, 2024

View reviewed changes

Tests/NetworkingTests/APIManagerTests.swift Outdated Show resolved Hide resolved

matejmolnar added 4 commits January 1, 2024 12:40

[feat] add multithread DownloadAPIManagerTests

2312444

[feat] change configuration

f2bff45

[chore] fix typo

9dd468a

[feat] add UploadAPIManagerTests

24b7b30

johnkodes reviewed Jan 2, 2024

View reviewed changes

Tests/NetworkingTests/DownloadAPIManagerTests.swift Outdated Show resolved Hide resolved

Tests/NetworkingTests/DownloadAPIManagerTests.swift Outdated Show resolved Hide resolved

Tests/NetworkingTests/UploadAPIManagerTests.swift Outdated Show resolved Hide resolved

matejmolnar and others added 2 commits January 2, 2024 17:22

Update Tests/NetworkingTests/UploadAPIManagerTests.swift

f54eb2b

Co-authored-by: Jan Kodeš <jan.kodes@strv.com>

Update Tests/NetworkingTests/DownloadAPIManagerTests.swift

2590c04

Co-authored-by: Jan Kodeš <jan.kodes@strv.com>

matejmolnar added 2 commits January 2, 2024 18:01

[fix] multithread managers tests

3481616

[fix] manager tests

c45f120

cejanen requested changes Jan 8, 2024

View reviewed changes

matejmolnar added 7 commits January 8, 2024 22:30

[chore] revert some classes back to open

dee8532

[chore] update file name comments

9645825

[chore] remove unnecessary explicit Sendable conformance

296c372

[chore] add final statement

987d49d

[feat] change class to open

d6613e0

[feat] remove unnecessary async statement

0b170b4

[fix] DownloadAPIManager tests by adjusting asyncResponse method

7b8c5df

matejmolnar requested a review from cejanen January 15, 2024 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat] Concurrency improvements #52

[feat] Concurrency improvements #52

matejmolnar commented Nov 27, 2023 •

edited

Loading

johnkodes left a comment •

edited

Loading

johnkodes Dec 17, 2023

matejmolnar Dec 18, 2023

johnkodes Dec 20, 2023

johnkodes Dec 21, 2023

matejmolnar commented Dec 30, 2023

johnkodes commented Jan 2, 2024

matejmolnar commented Jan 4, 2024

cejanen left a comment

cejanen Jan 7, 2024

matejmolnar Jan 8, 2024

cejanen Jan 7, 2024

matejmolnar Jan 11, 2024

cejanen Jan 7, 2024

matejmolnar Jan 11, 2024

cejanen Jan 8, 2024

matejmolnar Jan 14, 2024

cejanen Jan 8, 2024

matejmolnar Jan 14, 2024

johnkodes commented Jan 15, 2024

ipek commented Jan 15, 2024

[feat] Concurrency improvements #52

Are you sure you want to change the base?

[feat] Concurrency improvements #52

Conversation

matejmolnar commented Nov 27, 2023 • edited Loading

Changes:

johnkodes left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matejmolnar commented Dec 30, 2023

johnkodes commented Jan 2, 2024

matejmolnar commented Jan 4, 2024

cejanen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnkodes commented Jan 15, 2024

ipek commented Jan 15, 2024

matejmolnar commented Nov 27, 2023 •

edited

Loading

johnkodes left a comment •

edited

Loading