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

Replace withCheckedThrowingContinuation Calls With withUnsafeThrowingContinuation #4286

Merged
merged 4 commits into from
Sep 18, 2024

Conversation

fire-at-will
Copy link
Contributor

@fire-at-will fire-at-will commented Sep 18, 2024

Motivation

iOS 18 introduces a bug in Swift that causes calls to withCheckedThrowingContinuation to crash in certain scenarios. This PR provides some promise in addressing this issue, documented in #4177

Description

This PR replaces all calls to withCheckedThrowingContinuation to withUnsafeThrowingContinuation, which we're unable to reproduce the issue with.

Other Notes

withUnsafeThrowingContinuation doesn't provide any checking to ensure that the continuation isn't called multiple times, so this is something we'll need to be aware of going forward. Luckily, all existing usages of withCheckedThrowingContinuation were in just a few files and in small functions, so this is easy to check manually for now.

@fire-at-will fire-at-will added the pr:fix A bug fix label Sep 18, 2024
@fire-at-will
Copy link
Contributor Author

@RCGitBot please test

@fire-at-will fire-at-will marked this pull request as ready for review September 18, 2024 00:38
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.

We could probably follow up with a check to only use this version in iOS 18. Hopefully we wouldn’t have to live with it for too long in any case

return try await withCheckedThrowingContinuation { continuation in
return try await withUnsafeThrowingContinuation { continuation in
Copy link
Member

Choose a reason for hiding this comment

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

let’s add a comment pointing to the issue for future reference

@aboedo
Copy link
Member

aboedo commented Sep 18, 2024

@fire-at-will thanks for taking care of this!

@kristofferbv
Copy link

We’ve tested the branch, and it resolves the crash on launch. However, when opening the paywall, it crashes again, this time in withCheckedContinuation:
Skjermbilde 2024-09-18 kl  10 40 03
Skjermbilde 2024-09-18 kl  10 38 54

@aboedo
Copy link
Member

aboedo commented Sep 18, 2024

@kristofferbv thanks so much for reporting back! I've updated the branch to replace all instances of withCheckedContinuation -> withUnsafeContinuation, could you try it again and let us know if it does the trick?

@kristofferbv
Copy link

@kristofferbv thanks so much for reporting back! I've updated the branch to replace all instances of withCheckedContinuation -> withUnsafeContinuation, could you try it again and let us know if it does the trick?

Thanks! It works now with the latest changes. It would be really helpful if you could merge this branch into main.

@fire-at-will
Copy link
Contributor Author

@RCGitBot please test

@aboedo
Copy link
Member

aboedo commented Sep 18, 2024

@kristofferbv thanks for confirming! Merging to main now, will go out in the next release, likely today or tomorrow

@aboedo aboedo merged commit 41246f1 into main Sep 18, 2024
33 checks passed
@aboedo aboedo deleted the use-withUnsafeThrowingContinuation branch September 18, 2024 14:54
nyeu pushed a commit that referenced this pull request Oct 2, 2024
…Continuation (#4286)

* replace withCheckedThrowingContinuation calls with withUnsafeThrowingContinuation

* replaced instances of withCheckedContinuation -> withUnsafeContinuation

* update the links in comments to point to the issue not the PR

* linting

---------

Co-authored-by: andy boedo <andresboedo@gmail.com>
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.

3 participants