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

v4: Fix crash in iOS 11-12 when using MainActor #4718

Open
wants to merge 7 commits into
base: release/4.43.3
Choose a base branch
from

Conversation

MarkVillacampa
Copy link
Member

@MarkVillacampa MarkVillacampa commented Jan 27, 2025

Fix crash on iOS 11 and 12 when compiling with Xcode 16 and instantiating an array with type @mainactor lambda

var foo: [@MainActor @Sendable () -> Void] = []

@@ -27,7 +27,7 @@ class StoreKitRequestFetcher: NSObject {

private let requestFactory: ReceiptRefreshRequestFactory
private var receiptRefreshRequest: SKRequest?
private var receiptRefreshCompletionHandlers: [@MainActor @Sendable () -> Void]
private var receiptRefreshCompletionHandlers: [@Sendable () -> Void]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to understand, we shouldn't be using @MainActor, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exclusively in v4 of the iOS SDK. This is because it has a deployment target of iOS 11

Copy link
Member

@aboedo aboedo Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v5 is iOS 13+ so we can use it there

@manuelescrig
Copy link

manuelescrig commented Jan 30, 2025

I'd love if this can get merged. I'm using Version 4 of SDK on iOS 12 and when compiling it with Xcode 16 I'm getting this crash so I'm still compiling it with Xcode 15.

Copy link
Member

@aboedo aboedo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested and working on iOS 12, we should do a sanity pass to ensure we don't introduce any regression for newer devices and then ship!

Comment on lines 36 to 39
public typealias PurchaseCompletedBlock = @MainActor @Sendable (StoreTransaction?,
CustomerInfo?,
PublicError?,
Bool) -> Void
public typealias PurchaseCompletedBlock = @Sendable (StoreTransaction?,
CustomerInfo?,
PublicError?,
Bool) -> Void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed to remove this reference otherwise we'd still get a crash, since we have places where we store collections of these @MainActor references.

@aboedo
Copy link
Member

aboedo commented Feb 5, 2025

guess we don't get off that easy and I broke tests?

MarkVillacampa and others added 4 commits February 6, 2025 17:27
…ernal @mainactor block.

This way we avoid hitting the iOS 11 crash when initiailising a collection with a @mainactor block type, and we dont change the public interface.
…cessing the type metadata because the method is generic
Copy link
Member

@ajpallares ajpallares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just one question

CustomerInfo?,
PublicError?,
Bool) -> Void
public typealias PurchaseCompletedBlock = @MainActor @Sendable (StoreTransaction?,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one keeps the @MainActor. I'm guessing it's not used anymore in this v4. If so, perhaps we should remove it completely to prevent potential future errors if we need to make a new release of v4?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the concern around that one was about making further changes to the API - like, if your code relies on this being @MainActor, you might have broken compilation after upgrading.

I mean, the original problem was to add @MainActor in a non-minor anyway, but kinda don't want to repeat the same issue twice

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might as well kill the extra space so it doesn't even show up in the diff tho 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it makes sense 👍

CustomerInfo?,
PublicError?,
Bool) -> Void
public typealias PurchaseCompletedBlock = @MainActor @Sendable (StoreTransaction?,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the concern around that one was about making further changes to the API - like, if your code relies on this being @MainActor, you might have broken compilation after upgrading.

I mean, the original problem was to add @MainActor in a non-minor anyway, but kinda don't want to repeat the same issue twice

CustomerInfo?,
PublicError?,
Bool) -> Void
public typealias PurchaseCompletedBlock = @MainActor @Sendable (StoreTransaction?,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might as well kill the extra space so it doesn't even show up in the diff tho 😅

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

Successfully merging this pull request may close these issues.

6 participants