Swift 6: complete concurrency checking (LLC and UIKit) #3661
Conversation
# Conflicts: # Sources/StreamChat/WebSocketClient/BackgroundTaskScheduler.swift
# Conflicts: # Sources/StreamChat/Models/CurrentUser.swift # Sources/StreamChat/Workers/CurrentUserUpdater.swift
laevandus
added
the
🌐 SDK: StreamChat (LLC)
Generated by 🚫 Danger |
.github/workflows/smoke-checks.yml
Outdated
| if: ${{ github.event.inputs.record_snapshots != 'true' }} | ||
| # if: ${{ github.event.inputs.record_snapshots != 'true' }} | ||
| if: false # disable Xcode 15 builds |
There was a problem hiding this comment.
Disabled Xcode 15 builds (it is hard to support it when complete concurrency checking is enabled). Hard means avoiding compiler crashes.
SDK Size
|
| @@ -26,7 +26,7 @@ final class StreamAudioWaveformAnalyser: AudioAnalysing { | |||
| private let audioSamplesExtractor: AudioSamplesExtractor | |||
| private let audioSamplesProcessor: AudioSamplesProcessor | |||
| private let audioSamplesPercentageNormaliser: AudioValuePercentageNormaliser | |||
| private let outputSettings: [String: Any] | |||
| nonisolated(unsafe) private let outputSettings: [String: Any] | |||
There was a problem hiding this comment.
nonisolated(unsafe) because value is Any
SDK Performance
|
|
|
||
| import Foundation | ||
|
|
||
| /// Erase type for structs which recursively contain themselves. |
There was a problem hiding this comment.
It is easier to use a type instead of a closure when dealing with Sendable.
There was a problem hiding this comment.
yes, this is a nice approach
| @@ -131,15 +131,19 @@ open class StreamAudioPlayer: AudioPlaying, AppStateObserverDelegate { | |||
| open func play() { | |||
| do { | |||
| try audioSessionConfigurator.activatePlaybackSession() | |||
| player.play() | |||
| MainActor.ensureIsolated { | |||
| player.play() | |||
There was a problem hiding this comment.
Note that AVPlayer and some asset related methods require main actor
There was a problem hiding this comment.
why do we need this StreamConcurrency.onMain?
laevandus
changed the title
| static var maxAttachmentSize: Int64 { 100 * 1024 * 1024 } | ||
|
|
||
| private let decoder: RequestDecoder | ||
| private let encoder: RequestEncoder | ||
| private let session: URLSession | ||
| /// Keeps track of uploading tasks progress | ||
| @Atomic private var taskProgressObservers: [Int: NSKeyValueObservation] = [:] |
There was a problem hiding this comment.
did it complain about Atomic?
There was a problem hiding this comment.
Stored property '_taskProgressObservers' of 'Sendable'-conforming class 'StreamCDNClient' is mutable
Alternative is to use backwards compatible AllocatedUnfairLock (I added it for mutable static properties).
@Atomic is OK in many places because the class/type is @unchecked Sendable. In this particular case it is directly Sendable.
Property wrappers can't ensure concurrency safeness (there is a long thread about it on Swift forums)
There was a problem hiding this comment.
It does look cleaner:
/// Keeps track of uploading tasks progress
- private nonisolated(unsafe) var _taskProgressObservers: [Int: NSKeyValueObservation] = [:]
- private let queue = DispatchQueue(label: "io.getstream.stream-cdn-client", target: .global())
+ private let taskProgressObservers = AllocatedUnfairLock([Int: NSKeyValueObservation]())
init(
encoder: RequestEncoder,
@@ -149,13 +148,13 @@ final class StreamCDNClient: CDNClient, Sendable {
if let progressListener = progress {
let taskID = task.taskIdentifier
- queue.async {
- self._taskProgressObservers[taskID] = task.progress.observe(\.fractionCompleted) { [weak self] progress, _ in
+ taskProgressObservers.withLock { observers in
+ observers[taskID] = task.progress.observe(\.fractionCompleted) { [weak self] progress, _ in
progressListener(progress.fractionCompleted)
if progress.isFinished || progress.isCancelled {
- self?.queue.async { [weak self] in
- self?._taskProgressObservers[taskID]?.invalidate()
- self?._taskProgressObservers[taskID] = nil
+ self?.taskProgressObservers.withLock { observers in
+ observers[taskID]?.invalidate()
+ observers[taskID] = nil
}
}
}WDYT, let's use the snippet above?
There was a problem hiding this comment.
btw, we have this now in Stream Core, so you should align it, because that one will replace this.
| @@ -131,15 +131,19 @@ open class StreamAudioPlayer: AudioPlaying, AppStateObserverDelegate { | |||
| open func play() { | |||
| do { | |||
| try audioSessionConfigurator.activatePlaybackSession() | |||
| player.play() | |||
| MainActor.ensureIsolated { | |||
There was a problem hiding this comment.
mentioned it on the other PR as well - why do we need to use ensureIsolated?
Sources/StreamChat/Controllers/EventsController/EventsController.swift
Outdated
Show resolved
Hide resolved
| import os | ||
|
|
||
| @available(iOS, introduced: 13.0, deprecated: 16.0, message: "Use OSAllocatedUnfairLock instead") | ||
| final class AllocatedUnfairLock<State>: @unchecked Sendable { |
There was a problem hiding this comment.
where do we use an unfair lock?
There was a problem hiding this comment.
Only a couple of places in TestTools. If we prefer not to add this type, we can use some nonisolated(unsafe) in test tools (I am OK to do that).
There was a problem hiding this comment.
given that we use it until iOS 16, that might not be a bad idea (to remove it)
|
|
||
| import Foundation | ||
|
|
||
| /// Erase type for structs which recursively contain themselves. |
There was a problem hiding this comment.
yes, this is a nice approach
| try action() | ||
| } else { | ||
| try DispatchQueue.main.sync { | ||
| return try MainActor.assumeIsolated { |
There was a problem hiding this comment.
why is this needed?
There was a problem hiding this comment.
I can remove it, I think it was Xcode 15 which did not understand that this is OK (different compiler version). It is OK now.
| _endIndex = { baseCollection.endIndex } | ||
| _position = { baseCollection[$0] } | ||
| _startIndex = { baseCollection.startIndex } | ||
| self.baseCollection = Array(baseCollection) |
There was a problem hiding this comment.
why do we change this?
There was a problem hiding this comment.
Stored property '_endIndex' of 'Sendable'-conforming generic struct 'StreamCollection' has non-sendable type '() -> StreamCollection<Element>.Index' (aka '() -> Int')
At first I tried to make closures @Sendable, which requires BaseCollection to be Sendable. All good until I saw that StreamCollection is in some cases initialized with a list of MessageDTO. MessageDTO can't be Sendable, therefore I can't do this.
Fortunately we want to remove StreamCollection in v5 anyway.
There was a problem hiding this comment.
Reviewed it and don't have a better alternative to offer here.
| pongTimeoutTimer = timerType.schedule(timeInterval: Self.pongTimeoutTimeInterval, queue: timerQueue) { [weak self] in | ||
| log.info("WebSocket Pong timeout. Reconnect") | ||
| self?.delegate?.disconnectOnNoPongReceived() | ||
| queue.sync { |
There was a problem hiding this comment.
why is this needed?
There was a problem hiding this comment.
Problem here is that connectionDidChange is called from thread X and timer calls sendPing from thread Y which in turn will also access the _pingTimerControl (possible threading issue).
AllocatedUnfairLock would be more readable here.
Since we haven't heard about crashes with ping controller, I am open to keep it as is as well and not do these changes.
There was a problem hiding this comment.
is this some warning/error in Swift 6?
There was a problem hiding this comment.
I would prefer to not do any changes. We already have to do a lot of Swift 6 changes. The ones which are not 100% needed, we should not do them. To make reviewing easier, and also to have less risks when making the final release.
…here sync is required, use a LLC style DispatchQueue addition
… but where sync is required, use a LLC style DispatchQueue addition" This reverts commit e5b15f1.
# Conflicts: # Sources/StreamChat/Repositories/MessageRepository.swift
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 193 files out of 300 files are above the max files limit of 100. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
# Conflicts: # .github/workflows/smoke-checks.yml # Sources/StreamChat/Repositories/AuthenticationRepository.swift
# Conflicts: # Sources/StreamChat/APIClient/Endpoints/Payloads/MessagePayloads.swift # Sources/StreamChat/ChatClient+Environment.swift # Sources/StreamChat/Controllers/ChannelController/ChannelController.swift # Sources/StreamChat/Controllers/CurrentUserController/CurrentUserController.swift # Sources/StreamChat/Controllers/MessageController/MessageController.swift # Sources/StreamChat/Models/UserInfo.swift # Sources/StreamChat/Repositories/MessageRepository.swift # Sources/StreamChat/Workers/MessageUpdater.swift # TestTools/StreamChatTestTools/Mocks/StreamChat/ChatClient_Mock.swift
# Conflicts: # Sources/StreamChat/APIClient/ChatRemoteNotificationHandler.swift # Sources/StreamChat/Database/DTOs/MessageDTO.swift
| runs-on: macos-14 | ||
| if: ${{ github.event.inputs.record_snapshots != 'true' }} | ||
| runs-on: macos-15 | ||
| # if: ${{ github.event.inputs.record_snapshots != 'true' }} |
There was a problem hiding this comment.
what should we do here?
There was a problem hiding this comment.
Probably better to disable Xcode 15 for now, and once we add Xcode 26, we then put here Xcode 16 support. WDYT? @testableapple
| @@ -105,7 +107,9 @@ class DemoDraftMessageListVC: UIViewController, ThemeProvider { | |||
|
|
|||
| isPaginatingDrafts = true | |||
| currentUserController.loadMoreDraftMessages { [weak self] _ in | |||
| self?.isPaginatingDrafts = false | |||
| Task { @MainActor in | |||
There was a problem hiding this comment.
we should probably have a Swift 6 migration guide - customers would probably need to do the same things in their apps.
There was a problem hiding this comment.
Isn't there a way to mark the controllers as MainActor by default?
This is cleary a breaking change. IMO for supporting Swift 6, we should do a major bump of the SDK. Most libraries do this. It is an opportunity to do a light major version bump. We can remove deprecations and change minor things. For example, here we can remove the callback stuff, and just always dispatch the controller completion blocks to the main thread. I don't think there was every any customer changing the callback thread.
There was a problem hiding this comment.
Yeah, controller completions could be main actor if we would not have that callback queue property. At the moment there is no good way.
| resources: [.process("Fixtures")] | ||
| resources: [.process("Fixtures")], | ||
| swiftSettings: [ | ||
| .swiftLanguageMode(.v5) |
There was a problem hiding this comment.
why do we set this?
| static var maxAttachmentSize: Int64 { 100 * 1024 * 1024 } | ||
|
|
||
| private let decoder: RequestDecoder | ||
| private let encoder: RequestEncoder | ||
| private let session: URLSession | ||
| /// Keeps track of uploading tasks progress | ||
| @Atomic private var taskProgressObservers: [Int: NSKeyValueObservation] = [:] |
| static var maxAttachmentSize: Int64 { 100 * 1024 * 1024 } | ||
|
|
||
| private let decoder: RequestDecoder | ||
| private let encoder: RequestEncoder | ||
| private let session: URLSession | ||
| /// Keeps track of uploading tasks progress | ||
| @Atomic private var taskProgressObservers: [Int: NSKeyValueObservation] = [:] |
There was a problem hiding this comment.
btw, we have this now in Stream Core, so you should align it, because that one will replace this.
| import os | ||
|
|
||
| @available(iOS, introduced: 13.0, deprecated: 16.0, message: "Use OSAllocatedUnfairLock instead") | ||
| final class AllocatedUnfairLock<State>: @unchecked Sendable { |
There was a problem hiding this comment.
given that we use it until iOS 16, that might not be a bad idea (to remove it)
| struct StateBuilder<State> { | ||
| private let builder: (@MainActor() -> State) | ||
| struct StateBuilder<State: Sendable>: Sendable { | ||
| private var builder: ((@Sendable @MainActor() -> State))? |
There was a problem hiding this comment.
why is it optional?
There was a problem hiding this comment.
Yes why it is reuseOrBuild now? Can we add a comment explaining this?
| let state = builder!() | ||
| _state = state | ||
| // Release captured values in the closure | ||
| builder = nil |
There was a problem hiding this comment.
related to above - why do we need to nil it out?
| @@ -36,10 +36,12 @@ class IOSBackgroundTaskScheduler: BackgroundTaskScheduler { | |||
|
|
|||
| var isAppActive: Bool { | |||
| if Thread.isMainThread { | |||
| return app?.applicationState == .active | |||
| return MainActor.assumeIsolated { | |||
There was a problem hiding this comment.
why do we still need this here?
| pongTimeoutTimer = timerType.schedule(timeInterval: Self.pongTimeoutTimeInterval, queue: timerQueue) { [weak self] in | ||
| log.info("WebSocket Pong timeout. Reconnect") | ||
| self?.delegate?.disconnectOnNoPongReceived() | ||
| queue.sync { |
There was a problem hiding this comment.
is this some warning/error in Swift 6?
| runs-on: macos-14 | ||
| if: ${{ github.event.inputs.record_snapshots != 'true' }} | ||
| runs-on: macos-15 | ||
| # if: ${{ github.event.inputs.record_snapshots != 'true' }} |
There was a problem hiding this comment.
Probably better to disable Xcode 15 for now, and once we add Xcode 26, we then put here Xcode 16 support. WDYT? @testableapple
| @@ -105,7 +107,9 @@ class DemoDraftMessageListVC: UIViewController, ThemeProvider { | |||
|
|
|||
| isPaginatingDrafts = true | |||
| currentUserController.loadMoreDraftMessages { [weak self] _ in | |||
| self?.isPaginatingDrafts = false | |||
| Task { @MainActor in | |||
There was a problem hiding this comment.
Isn't there a way to mark the controllers as MainActor by default?
This is cleary a breaking change. IMO for supporting Swift 6, we should do a major bump of the SDK. Most libraries do this. It is an opportunity to do a light major version bump. We can remove deprecations and change minor things. For example, here we can remove the callback stuff, and just always dispatch the controller completion blocks to the main thread. I don't think there was every any customer changing the callback thread.
| Task { @MainActor in | ||
| self?.updateData() | ||
| } |
There was a problem hiding this comment.
Yeah, we can't put this everywhere... if customers require doing this, this a breaking change. Will this produce a warning or an error?
There was a problem hiding this comment.
Warning if they use swift 5, error when swift 6
| resources: [.process("Fixtures")] | ||
| resources: [.process("Fixtures")], | ||
| swiftSettings: [ | ||
| .swiftLanguageMode(.v5) |
| ) { | ||
| // it's worth noting here that according to the documentation, the completion | ||
| // handler will be invoked only once, regardless of the number of | ||
| // properties we are loading. | ||
| // https://developer.apple.com/documentation/avfoundation/avasynchronouskeyvalueloading/1387321-loadvaluesasynchronously | ||
|
|
||
| // On iOS 15 we should switch to async load methods |
| struct StateBuilder<State> { | ||
| private let builder: (@MainActor() -> State) | ||
| struct StateBuilder<State: Sendable>: Sendable { | ||
| private var builder: ((@Sendable @MainActor() -> State))? |
There was a problem hiding this comment.
Yes why it is reuseOrBuild now? Can we add a comment explaining this?
| pongTimeoutTimer = timerType.schedule(timeInterval: Self.pongTimeoutTimeInterval, queue: timerQueue) { [weak self] in | ||
| log.info("WebSocket Pong timeout. Reconnect") | ||
| self?.delegate?.disconnectOnNoPongReceived() | ||
| queue.sync { |
There was a problem hiding this comment.
I would prefer to not do any changes. We already have to do a lot of Swift 6 changes. The ones which are not 100% needed, we should not do them. To make reviewing easier, and also to have less risks when making the final release.
| private let queue = DispatchQueue(label: "io.getstream.event-notification-center-sync", target: .global()) | ||
| private(set) var middlewares: [EventMiddleware] { | ||
| get { queue.sync { _middlewares } } | ||
| set { queue.sync { _middlewares = newValue } } |
There was a problem hiding this comment.
why not async? We should try to use async whenever possible
| private(set) var newMessageIds: Set<MessageId> = Set() | ||
| private(set) var newMessageIds: Set<MessageId> { | ||
| get { queue.sync { _newMessageIds } } | ||
| set { queue.sync { _newMessageIds = newValue } } |
| queue.sync { | ||
| _middlewares.append(contentsOf: middlewares) | ||
| } |
|
I am going to make breaking changes what we discussed separately, take all the feedback from this PR, and later create a brand new PR. |
Important
StreamChatUI changes are in #3660 and will be merged to here. This PR can be reviewed already.
🔗 Issue Links
Resolves IOS-735
🎯 Goal
📝 Summary
Sendableconformance to types@Sendableto completion handlers@unchecked Sendabletypes (internal classes have manual handling and found many which did not guard that state properly)@Sendableto completion handlers creates warnings at callsites)🛠 Implementation
🧪 Manual Testing Notes
Manual regression testing round when UIKit changes are merged into this branch.
☑️ Contributor Checklist
docs-contentrepo