SDKS-3458 Swift6 adoption + concurrency check #6
Conversation
jeyanthanperiyasamy
force-pushed
the
SDKS-3458
branch
from
2b52e60 to
9d05817
Compare
jeyanthanperiyasamy
requested review from
vahancouver,
spetrov and
george-bafaloukas-forgerock
jeyanthanperiyasamy
force-pushed
the
SDKS-3458
branch
from
477e831 to
598813d
Compare
|
|
||
| import Foundation | ||
|
|
||
| public struct SendableAny: @unchecked Sendable { |
There was a problem hiding this comment.
Add doc to the struct and public methods
jeyanthanperiyasamy
force-pushed
the
SDKS-3458
branch
3 times, most recently
from
41794d3 to
9c6721f
Compare
| let components = form[Constants.components] as? [String: Any], | ||
| let fields = components[Constants.fields] as? [[String: Any]] { | ||
| collectors = CollectorFactory().collector(from: fields) | ||
| actor Form { |
There was a problem hiding this comment.
There is not mutable state in Form, any reason we need to define it as actor?
There was a problem hiding this comment.
Form can be a struct ... we generally reduce using classes. For this scenario actor is not required. i will change this.
| public var key: String = "" | ||
| public var label: String = "" | ||
| public var value: String = "" | ||
| open class FieldCollector: Collector, @unchecked Sendable { |
There was a problem hiding this comment.
Looks like you handle concurrent update of attributes, why define as unchecked?
There was a problem hiding this comment.
In Swift 6, this code won't compile as-is, because the compiler will error:
"Non-final class 'FieldCollector' cannot conform to 'Sendable'".
To resolve this, we need to instruct the compiler to skip the compatibility check by explicitly marking the conformance as unchecked. This can be done using @unchecked Sendable,
|
|
||
| func reset() { | ||
| collectors.removeAll() | ||
| public actor CollectorFactory { |
There was a problem hiding this comment.
yes, CollectorFactory.shared is the usage.
So customer will have to call like to add custom callback ..
await CollectorFactory.shared.register(type: collector:)
| @@ -13,7 +13,7 @@ import Foundation | |||
| import PingOidc | |||
| import PingOrchestrate | |||
|
|
|||
| public class OidcModule { | |||
| public actor OidcModule { | |||
There was a problem hiding this comment.
There is no state in OidcModule, why define it as actor?
There was a problem hiding this comment.
by default static property in classes are not thread safe. so compiler will throw error
Static property 'config' is not concurrency-safe because non-'Sendable' type 'Module<OidcClientConfig>' may have shared mutable state
There was a problem hiding this comment.
Let me think about this in another way
| @@ -10,7 +10,7 @@ | |||
|
|
|||
|
|
|||
| /// Class for an OIDC User | |||
| public class OidcUser: User { | |||
| public final class OidcUser: User, @unchecked Sendable { | |||
There was a problem hiding this comment.
To convert this to actor , we need to convert lot of other code to be sendable like AgentDelegate, OidcClient etc to be sendable .
Since this is an first step for swift 6. We should continuously seek opportunities to reduce class usage as much as possible and use actor/struct . so we are better compatibile with Swift 6 concurrency
jeyanthanperiyasamy
force-pushed
the
SDKS-3458
branch
from
9c6721f to
0896ae4
Compare
jeyanthanperiyasamy
force-pushed
the
SDKS-3458
branch
from
0896ae4 to
947f55d
Compare
JIRA Ticket
SDKS-3458 DaVinci Swift 6.0 adoption (concurrency check)
Description
Reduce the reliance on traditional classes as part of our commitment to improve compatibility with Swift 6 concurrency. This initial commit marks a step in that direction.
To achieve better thread safety, we should prioritize using actor, struct, and final class more frequently, while minimizing inheritance where possible. Embracing a concurrency-first pattern will help us leverage Swift’s capabilities more effectively.
We should continuously identify opportunities to decrease class usage, ensuring our code is both safe and efficient in a concurrent environment.
Checklist: