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 issues in ImageDownload and ImageDownloader #3072
Conversation
- Fix a lot of thread-safety issues. - Fix incorrect handling of errors: - Completions are called two times for HTTP status code >= 400. - `ImageDownload` operation wasn't properly transition into finished state. - `ImageDownload` operation didn't have proper `isReady` state. - Increase test coverage for these types.
16e680b
to
367eb67
Compare
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.
Locking internal to a class is always scary, but I think you’ve covered your bases.
// So we trigger `init` here so that `urlSession` is initialized in a thread safe manner. | ||
_ = urlSession |
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.
Does the lazy syntax benefit us here? Maybe it’d be simpler to inline that initialization here.
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.
Good questions, thanks for asking. Without going deep into explaining the rationale why I've kept lazy var
I think we can change private lazy var urlSession: URLSession
to private var urlSession: URLSession!
.
There is no need for dynamic dispatch. This class doesn't have subclasses and not intended to have.
Sorry, can you please rephrase? |
Fixes #3067
ImageDownload
operation wasn't properly transition into finishedstate.
ImageDownload
operation didn't have properisReady
state.Implementation
ImageDownloader
ImageDownloader
is fixed by properly using Dispatch Queue: perform protected state read/write inside the queue.accessQueue
replaced by a serial one, because most accesses to the queue modifies the protected state that require a barrier to be placed. That makes theaccessQueue
merely a serial queue.ImageDownload
ImageDownload
is a subclass of NSOperation.State
variable. No more multiple flags juggling.objc_sync_enter
with a lock.