-
Notifications
You must be signed in to change notification settings - Fork 16
1601 - Crash on createTileArrays() - Not Thread Safe #167
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
Conversation
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.
One small note to think about, but code looks good.
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.
This solution is not the right solution.
We have a concurrency bug that I documented with multiple threads accessing the same shared data in the createTileOverlays()
method. Changing the type of the published property might punt the crash and doesn't solve anything about concurrent access. It might appear to be less likely to crash, but it is still just as unsafe.
Both Task 10 and Task 11 are modifying the same shared resource from concurrent queues. This is unsafe code to be modified by both Tasks concurrently.
We need to protect with either actor, serial queue, or static @mainactor protections to prevent modifying shared state from two different threads at the same time.

d58ffdf
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.
Looks better. I'm not sure what approach actually solves the concurrency issue.
It almost feels like it's because the Task
might be on a background thread inside the .sink
. So that we'd need await MainActor.run {
to do the main thread work on the proper thread.
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.
Looks good. I have not seen a crash on the simulator or iPhone 15 Pro Max running on this branch.
The new changes should make this logic safer. In the future we can explore more @mainactor changes, or make it the default in Swift 6.2 with Xcode 26.
Uh oh!
There was an error while loading. Please reload this page.