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.
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
Fix Sendable Warnings with Strict Concurrency Checking #215
base: strictConcurrencyCompileErrorsFix
Are you sure you want to change the base?
Fix Sendable Warnings with Strict Concurrency Checking #215
Conversation
@@ -2,8 +2,8 @@ import Foundation | |||
|
|||
/// Constructs `AnalyticsEventData` models and sends FPTI analytics events. | |||
@_documentation(visibility: private) | |||
public struct AnalyticsService { | |||
|
|||
public struct AnalyticsService: @unchecked Sendable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious about the reason behind using @unchecked here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With @unchecked, the Sendable compiler warnings are silenced, by marking this class as @unchecked Sendable I am assuring that the type is safe to pass between concurrent contexts. Without it, I get a warning on Sendable capture in sendEvent
function awaiting performEventRequest
. This is because the Task block creates an asynchronous context. Since we are just sending data to analytics endpoint, I don't think there is danger of data races. The values of coreConfig
and orderID
are not changing after initialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about making CoreConfig
and TrackEventsAPI
conform to Sendable
? Would this be more maintainable over time since any new updates could introduce something that is unsafe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be a good thing to try! We do run into kind of dead end down the road when we hit something like urlSession property in HTTP
. I will put up a commit. A LOT of Sendables slapped on everywhere for the types of properties and the classes need to be final.
Is this PR still active? Or can we close it |
Reason for changes
This PR addresses compiler warnings related to strict concurrency checking option set to complete.
This PR is dependent on #214 which addresses reason for moving toward more concurrency safety, adhering to stricter concurrency checking in Xcode.
Sendable
protocol in Swift is part of the concurrency model introduced to ensure thread safety. It is a way of marking types that are safe to transfer across concurrent execution contexts. AdoptingSendable
protocol is a guarantee to the compiler that instances of this type can be safely shared between different threads without risk of data race.A type is
Sendable
ifSendable
types.Sendable
, and the class itself ensures proper synchronization for its mutable state.By default, simple value types like "Int", "String", and "Array" (when they contain
Sendable
types) conform toSendable
. For custom types, developers need to ensure thread safety and then mark their types asSendable
to inform both the compiler and other developers that these types can be safely used in concurrent code.Sendable protocol does not by itself enforce any runtime checks or provide any thread-safety features. Instead, it's a static type system feature that helps the compiler verify and enforce the safety of concurrent code at compile time.
This PR's fixes addressed "non-sendable type cannot cross actor boundary" warning and "capture of non-sendable type" in Task blocks with types that don't conform to
Sendable
"non-sendable type cannot cross actor boundary"
Actors
in Swift are a feature that provides a way to protect shared mutable state by serializing access to the state they encapsulate. When the warning says "non-sendable type cannot cross actor boundary", it's indicating that you're trying to use a type that hasn't been marked as safe for concurrent use (Sendable
) in an actor, which is designed to work withSendable
types only. This could potentially lead to unsafe concurrent access to the actor's state."capture of non-sendable type"
Task blocks in Swift are constructs that allow you to perform work asynchronously. When you use a task block, you're creating a new asynchronous context. This means that the code within the task block can run concurrently with other code. Because of this concurrency, we need to make sure that any data we access within a task block is safe to use from multiple threads or execution contexts. This is the reason for capture warnings for non-Sendable types within the Task block.
Unchecked Sendable
The @unchecked Sendable conformance is a way to suppess the compiler's warnings about a type not being
Sendable
.Using this is a promise that usage of the type is safe, even though not all the enclosed types conform to
Sendable
I'm open to discussions about how strict we want to be about these warnings. I think it's a balance between amount of code change and blanket hand waving, assuring that everything is handled correctly to avoid data race.
Summary of changes
Made requests (CardRequest, CardVaultRequest and all the nested types contained) conform to
Sendable
Added @unchecked
Sendable
conformance toAnalyticsService
to suppressSendable
capture warning inside Task block insendEvent
function.Marking it
Sendable
without @unchecked gave warning about its properties,TrackingEventsAPI
,CoreConfig
not beingSendable
.There are so many types to be marked
Sendable
without @unchecked. I did find that http private property inNetworkingClient
, and urlSession in HTTP class don't need to be vars. To make HTTP conform to Sendable, the class has to be made final. And there is no fix for "Stored property 'urlSession' of 'Sendable'-conforming class 'HTTP' has non-sendable type 'any URLSessionProtocol'" error on our end. In short, we can make this justSendable
, not @uncheckedSendable
but a lot more code change.Moved @mainactor annotation from top of
TrackingAPI_Tests
because of warnings onsendEvent
calls.Unfixed warnings:
CardClient
inside Task blocks of approve and vault functions ( they are silenced with @uncheckedSendable
on CardClient). I would have to mark this as @uncheckedSendable
as well because of delegate weak references. ( warning - Stored property 'delegate' of 'Sendable'-conforming class 'CardClient' is mutable )I would also need to make
CardClient
final to conform toSendable
“Main actor-isolated instance method 'presentationAnchor(for:)' cannot be used to satisfy nonisolated protocol requirement”.
Checklist
- [ ] Added a changelog entryAuthors