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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix] Change on-demand resources downloaded state criteria #208

Merged
merged 1 commit into from
Jun 21, 2023

Conversation

yo1995
Copy link
Collaborator

@yo1995 yo1995 commented Jun 20, 2023

Description

This PR fixes the ODR resource force unwrap not found problem described in #48

Linked Issue(s)

  • swift/issues/4199

How To Test

  1. Do a fresh install and fresh build
  2. When the samples app first open, check an arbitrary sample that has ODR, e.g.
  3. Make sure none of them crashes

To Discuss

I haven't been able to reproduce a crash with this fix. My gut says the root cause was that when NSBundleResourceRequest sets its progress to 100%, it is not immediately setting the isResourceAvailable to true, or it is dispatched onto the main serial queue whereas the state change is propagated to the view earlier than that. 馃

  • One way to fix is to move the requestState = .downloaded to a later point.

According to the doc for beginAccessingResources(completionHandler:),

The resources are not available until the completion handler is called with error set to nil.

So when the try await request.beginAccessingResources() call returns, the resource must be available.

  • Another potential way to fix is to call the download() method again after the progress reported 100%, so it conditionally checks for the resource again.

@yo1995 yo1995 linked an issue Jun 20, 2023 that may be closed by this pull request
@yo1995 yo1995 self-assigned this Jun 20, 2023
@yo1995 yo1995 changed the title [Fix] Change on-demand resources downloaded state criteria. [Fix] Change on-demand resources downloaded state criteria Jun 20, 2023
Copy link
Contributor

@dfeinzimer dfeinzimer left a comment

Choose a reason for hiding this comment

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

I was unable to produce a crash on v.next to validate but was unable. Changes look okay.

@yo1995
Copy link
Collaborator Author

yo1995 commented Jun 21, 2023

Thank you! I was able to crash twice on v.next with random order of accessing these ODR samples in about 10 fresh installations, and haven't crashed yet with the fix. Will merge and keep an eye on any future crashes if there is any. 馃憖

@yo1995 yo1995 merged commit 2a01137 into v.next Jun 21, 2023
1 check passed
@yo1995 yo1995 deleted the Ting/Fix-ODR-timing branch June 21, 2023 00:28
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.

Fix raster file download problem in Add raster from file
3 participants