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.

Sign up for GitHub

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

Jump to bottom

Improve Swift Concurrency support #1041

Closed
aboedo opened this issue Dec 7, 2021 · 16 comments
Closed

Improve Swift Concurrency support #1041

aboedo opened this issue Dec 7, 2021 · 16 comments
Assignees
@NachoSoto
Labels
enhancement triaged

Comments

@aboedo
Copy link
Member

aboedo commented Dec 7, 2021 •
edited
Loading

We're using Swift Concurrency in a few places.
There are a couple of compiler flags that help prevent issues with Swift Concurrency, that we should leverage.

The flags are
-Xfrontend -warn-concurrency -Xfrontend -enable-actor-data-race-checks

And can be enabled in Xcode -> RevenueCat Project -> RevenueCat target -> Build Settings -> Other Swift Flags.

image

Enabling them fires up a few warnings and errors.
We should enable the compiler flags, and fix the warnings / errors.
More context here: https://forums.swift.org/t/concurrency-in-swift-5-and-6/49337

https://app.shortcut.com/revenuecat/story/11686/enable-swift-compiler-flags-for-concurrency-and-fix-errors
@NachoSoto
Copy link
Contributor

NachoSoto commented Dec 10, 2021 •
edited
Loading

From the post:

If at any point the only way forward for the developer is to switch the language mode and fix 100 errors before they can make progress, we have failed.

Well that's kind of the case right now with these warnings. I worked on this for a while yesterday, and it's kinda of a pain to do right in a codebase that only partially supports Swift concurrency. A bunch of types in the standard library (Foundation, UIKit, etc) added @MainActor (which, I have to say, it's a great abstraction for being able to annotate "this class isn't thread safe", but...), which requires us to either:

  • Make our own types @MainActor
  • await calls that come from outside the actor.

The second option leads to my biggest pet-peeve of async/await: an explosion of everything needing to be async. FRP doesn't suffer from this as much. In our case not only does that require making lots of code asynchronous, but it's also impossible because we need to support pre-iOS 13.0.
The first option works, but only delays the need for the second one further up the hierarchy.

Similarly, declarations can be marked with the “unsafe” form of a global actor, meaning that they should be considered to be a part of the global actor, but that should only be enforced in Swift 6 code or Swift 5 code that has adopted concurrency. For example:

Unfortunately @MainActor(unsafe) isn't helping us here either. It's not clear what "Swift 5 code that has adopted concurrency." refers to, but adding @MainActor(unsafe) to a method that uses a @MainActor type, and calling that method from a regular method still fails:

Property 'identifierForVendor' isolated to global actor 'MainActor' can not be referenced from this synchronous context

There's also problems with the need for APIs to have @Sendable closures, which the post talks about. @_unsafeSendable would help here, but that's not available yet.

There's a few improvements that I'll put in a PR in a bit, but for the most part unfortunately there isn't much that we can do for the time being.
I say for the time being, but I'm scared of what this situation will look like in Swift 6.0.

@aboedo
Copy link
Member Author

aboedo commented Dec 10, 2021 •
edited
Loading

🤕 Xcode 14 will be a fun release. Thanks for taking a look at this!

@NachoSoto
Copy link
Contributor

Good news: https://twitter.com/dgregor79/status/1470940523259056129?s=21

@taquitos
Copy link
Contributor

taquitos commented Jan 6, 2022

Won't fix until Swift 5.6

@taquitos taquitos added the HOLD label Jan 6, 2022
@pro-akbar
Copy link

I am still facing this type of error

@joshdholtz
Copy link
Member

@pro-akbar Hey! Would you be able to create a new issue and fill out all of your environment information in there? 😊 It's a bit easier to have a conversation in a new issue and reference this one. Thank you thank you! ❤️

@AvdLee
Copy link

AvdLee commented Aug 5, 2022 •
edited
Loading

I think it's a good moment for this SDK to start preparing for Swift 6 and adopt Sendable where possible.

It should be doable to add Sendable inheritance for some types. I've currently got these warnings:

CleanShot 2022-08-05 at 15 13 03@2x

@aboedo
Copy link
Member Author

aboedo commented Aug 5, 2022

@AvdLee thanks for bringing this up!

We tried it out a while back but had some issues that we would hoping would go away in newer Xcode betas. We'll give it another shot.

@NachoSoto NachoSoto mentioned this issue Aug 5, 2022
Merged
@NachoSoto
Copy link
Contributor

@AvdLee Sendable support should be ready in #1795. Would you be able to give that branch a try?
If not we could release that as a beta for you to try it in your app.

@joshdholtz
Copy link
Member

Not relevant to this issue but just wanted to say... Hi @AvdLee! 👋

NachoSoto added a commit that referenced this issue Aug 31, 2022
@NachoSoto
Sendable support (#1795)
0bff3f6 
Fixes [CSDK-379] and #1041 (comment)

[CSDK-379]: https://revenuecats.atlassian.net/browse/CSDK-379?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

### Main changes:
- `Sendable` conformances to all types that are thread-safe
- `@unchecked Sendable` for types that the compiler can't enforce, but with documentation as to why
- Made classes that aren't mocked `final` (note that this doesn't change the API, because none of these were `open` to begin with)
- Made some classes thread-safe that weren't (like `DeviceCache`)

For the non-`final` `class`es, I've actually managed to compile the SDK with all of them as `final` and `Sendable`, to make sure that making them `@unchecked Sendable` didn't hide any other issues.

### Future improvements:
- https://twitter.com/nachosoto/status/1557139992056500224?s=21&t=arWdvEzTIFANBvwqQ0vPiA
- https://twitter.com/nachosoto/status/1557141944777592832?s=21&t=arWdvEzTIFANBvwqQ0vPiA
- https://twitter.com/nachosoto/status/1557143374922059776?s=21&t=arWdvEzTIFANBvwqQ0vPiA

### Depends on:
- #1794
- #1804
- #1806
- #1807
- #1808
- #1813
- #1822
- #1823
- #1824
- #1825
- #1826
@NachoSoto
Copy link
Contributor

Should we close this? :)

@aboedo
Copy link
Member Author

aboedo commented Aug 31, 2022

I think we're all done with this one! Closing

@aboedo aboedo closed this as completed Aug 31, 2022
@NachoSoto NachoSoto self-assigned this Aug 31, 2022
@aboedo
Copy link
Member Author

aboedo commented Aug 31, 2022

Changes will be released in the next version, next Wednesday or sooner

@AvdLee
Copy link

AvdLee commented Sep 15, 2022

Thanks a lot for the hard work on this @NachoSoto! And Hi back to you @joshdholtz ⛳️

@joshdholtz
Copy link
Member

Wow... always rubbing in that you won that round of golf, aren't you? 😛

@AvdLee
Copy link

AvdLee commented Sep 19, 2022

Of course, I need to create extra reason to meet up again for another round of golf 🤪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@NachoSoto NachoSoto

Labels
enhancement triaged
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@joshdholtz @NachoSoto @taquitos @aboedo @AvdLee @pro-akbar