Skip to content
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

Open
wants to merge 1 commit into
base: strictConcurrencyCompileErrorsFix
Choose a base branch
from

Conversation

KunJeongPark
Copy link
Collaborator

@KunJeongPark KunJeongPark commented Nov 2, 2023

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. Adopting Sendable 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 if

  1. It is a value type (like a struct or enum) that only contains Sendable types.
  2. It's a class where all properties are 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 to Sendable. For custom types, developers need to ensure thread safety and then mark their types as Sendable 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 with Sendable 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 to AnalyticsService to suppress Sendable capture warning inside Task block in sendEvent function.

    Marking it Sendable without @unchecked gave warning about its properties, TrackingEventsAPI, CoreConfig not being Sendable.

    There are so many types to be marked Sendable without @unchecked. I did find that http private property in NetworkingClient, 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 just Sendable, not @unchecked Sendable but a lot more code change.

  • Moved @mainactor annotation from top of TrackingAPI_Tests because of warnings on sendEvent calls.

Unfixed warnings:

  • capture warning on CardClient inside Task blocks of approve and vault functions ( they are silenced with @unchecked Sendable on CardClient). I would have to mark this as @unchecked Sendable 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 to Sendable
  • warning with @mainactor annotation with presentationAnchor function:
    “Main actor-isolated instance method 'presentationAnchor(for:)' cannot be used to satisfy nonisolated protocol requirement”.

Checklist

- [ ] Added a changelog entry

Authors

List GitHub usernames for everyone who contributed to this pull request.

@@ -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 {
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

@scannillo
Copy link
Collaborator

Is this PR still active? Or can we close it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants