Swift 6: complete concurrency checking for StreamChatUI #3660
Conversation
# Conflicts: # Tests/StreamChatUITests/SnapshotTests/ChatChannelList/Search/ChatMessageSearchVC_Tests.swift # Tests/StreamChatUITests/SnapshotTests/ChatMessageList/ChatMessage/ChatMessageMarkdown_Tests.swift
laevandus
added
🎨 SDK: StreamChatUI
Generated by 🚫 Danger |
SDK Size
|
| @@ -15,7 +15,7 @@ open class DefaultChannelNameFormatter: ChannelNameFormatter { | |||
| public init() {} | |||
|
|
|||
| /// Internal static property to add backwards compatibility to `Components.channelNamer` | |||
| internal static var channelNamer: ( | |||
| nonisolated(unsafe) internal static var channelNamer: ( | |||
There was a problem hiding this comment.
TODO: Review if MainActor is more approriate here
SDK Performance
|
| static var `default`: Appearance = .init() | ||
| static var `default`: Appearance { | ||
| get { queue.sync { _default } } | ||
| set { queue.sync { _default = newValue } } | ||
| } | ||
|
|
||
| private static let queue = DispatchQueue(label: "io.getstream.appearance", target: .global()) | ||
| nonisolated(unsafe) private static var _default: Appearance = .init() |
There was a problem hiding this comment.
On the second look it actually does not feel like a good idea. Since this is used for UI code then it sounds like making the static property MainActor makes more sense here. Otherwise we do unnecessary context switches. Making it main actor will have consequences:
- app level setup code must be done on the main thread
- other code interacting with default must require main thread
There was a problem hiding this comment.
In general, options are:
a) opt-out with nonisolated(unsafe) which kind of loses the point of strict concurrency checking. SDK users can trigger crashes.
b) Internally uses main thread when accessing the default (which should be the 99% of cases when calling it)
static var `default`: Appearance {
get {
MainActor.ensureIsolated { _default }
}
set {
MainActor.ensureIsolated { _default = newValue }
}
}and making Appearance @unchecked Sendable (lot of work to make it fully Sendable since this type captures so many other types, e.g formatters)
c) Make the default @MainActor and add MainActor requirement to all the other callsites
e) Use queue/lock for accessing the default state although it should (almost) always be main thread
There was a problem hiding this comment.
a) loses the point of concurrency checking
b) makes it easier to use default from places without main actor annotation (easier adoption), on the other hand, it does not look the nicest, but at least does not cause performance issues due to context switches
c) cleanest but harder to use for users since it forces MainActor
e) performance wise not good because this is used by UI code and should be almost always run on the main thread (that said, SDK users could have setup running on a background thread)
| self.configurable = configurable | ||
| self.defaultValue = defaultValue | ||
| } | ||
| } | ||
|
|
||
| extension PollsEntryConfig { | ||
| /// The default configuration for a poll entry. It will make it configurable but disabled by default. | ||
| public static let `default` = PollsEntryConfig(configurable: true, defaultValue: false) | ||
| // TODO: Uncomment later | ||
| public static var `default`: PollsEntryConfig { PollsEntryConfig(configurable: true, defaultValue: false) } |
There was a problem hiding this comment.
Not sure why, but compiler crashes with Xcode 15 when using default for init above. The only way I found was to skip using default and set the values in the PollEntryConfig. Compiles fine with Xcode 16.
| [dateView, unreadCountView].forEach(container.addArrangedSubview) | ||
| [dateView, unreadCountView].forEach { container.addArrangedSubview($0) } |
There was a problem hiding this comment.
Xcode 15 fails to compile such code when complete concurrency checking is enabled (stack was not useful) OK with Xcode 16.
| ) { | ||
| self.listView = listView | ||
| self.impactFeedbackGenerator = impactFeedbackGenerator | ||
| self.impactFeedbackGenerator = impactFeedbackGenerator ?? UIImpactFeedbackGenerator(style: .medium) |
There was a problem hiding this comment.
Xcode 15 had compiler crashes and this was the only way I could get it compiling
SDK Size
|
| static var `default`: Appearance = .init() | ||
| static var `default`: Appearance { | ||
| get { | ||
| MainActor.ensureIsolated { _default } | ||
| } | ||
| set { | ||
| MainActor.ensureIsolated { _default = newValue } | ||
| } | ||
| } | ||
|
|
||
| // Shared instance is mutated only on the main thread without explicit | ||
| // main actor annotation for easier SDK setup. | ||
| nonisolated(unsafe) private static var _default: Appearance = .init() |
There was a problem hiding this comment.
At first I was thinking about making it @MainActor, but this is used in multiple init methods meaning all of these must be @MainActor as well. Moreover, it might make the SDK initialization more complex as well since SDK users need to make sure Appearance is changed only from the MainActor.
a) we just make default nonisolated(unsafe)
b) we do what we have above which internally forcing main actor (for making sure SDK users do not corrupt the default by changing it from background threads)
|
Important
Merges to the main
swift-6branch, notdevelop🔗 Issue Links
Resolves IOS-735
🎯 Goal
📝 Summary
@Sendablein most of the casesMainActor.ensureIsolated)nonisolatedbecause any thread thread could call it and often MainActor guarded type implements these delegates (e.g. UIViewController subclass)nonisolated(unsafe)a lot because otherwise most of the test must be rewritten (huge effort)@MainActorrequirement🛠 Implementation
LLC controllers support any queue for delegate callbacks and completion handlers. Therefore we need to ensure that these run on the main thread when updating UI code. This is where the custom
MainActor.ensureIsolated(_)comes into the play. It just ensures that code triggered from these code paths are always on the main thread.Public API which requires MainActor are annotated with
@preconcurrency @MainActorwhich is the backwards compatibility support. In the next major version we would clean it up.🧪 Manual Testing Notes
Manual testing in #3661
☑️ Contributor Checklist
docs-contentrepo