-
Notifications
You must be signed in to change notification settings - Fork 976
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
Fix synchronous download manager #4186
base: master
Are you sure you want to change the base?
Conversation
🚨 New dead code detected in this PR: DownloadManager.swift:108 warning: Function 'resetCache()' is unused Please remove the dead code before merging. If this is intentional, you can bypass this check by adding the label ℹ️ If this comment appears to be left in error, double check that the flagged code is actually used and/or make sure your branch is up-to-date with |
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.
If only we could make DownloadManager
an actor
🥲
Agreed @mats-stripe! Some discussion here: https://stripe.slack.com/archives/CFA2HJ99A/p1729811584450199?thread_ts=1729805357.152749&cid=CFA2HJ99A |
Summary
Deleted the synchronous path of code in DownloadManager; this is unsafe (as the comment indicates!) and can deadlock.
The purpose of the synchronous code was, I believe, to make the method return the cached image by synchronously making a network request that would then access the URLSession cache. The better way to do this is to maintain an in-memory cache, implemented in this PR.
I tried to make a minimal change here to fix the deadlock; broader refactor effort is needed and ticketed here: https://jira.corp.stripe.com/browse/MOBILESDK-2604
Testing
See tests
Changelog
See changelog