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

ImageContainer#cancelRequest is unsafe to call off the main thread #132

Closed
jpd236 opened this issue Dec 21, 2017 · 1 comment
Closed

ImageContainer#cancelRequest is unsafe to call off the main thread #132

jpd236 opened this issue Dec 21, 2017 · 1 comment
Milestone

Comments

@jpd236
Copy link
Collaborator

jpd236 commented Dec 21, 2017

As noted in #130, calling cancelRequest may mutate mBatchedResponses at the same time that it is being read in ImageLoader#batchResponse's inner Runnable, which is queued on a main thread handler.

Need to take a closer look at the thread safety guarantees here and either fix them or add documentation on constraints (e.g. only cancel requests on the main thread).

@jpd236 jpd236 added this to the 1.1.1 milestone Dec 21, 2017
@jpd236
Copy link
Collaborator Author

jpd236 commented May 4, 2018

ImageLoader (parent class of ImageContainer) has always had Javadoc noting that all calls must be made from the main thread. So I think it's reasonable to enforce this in 1.1.1, even though it may technically break some users, because doing so would have been a clear application bug.

This class is making an assumption though that response delivery happens on the main thread, which is generally true for any ResponseDelivery implementation (including the default), but isn't a guarantee. We could generalize this, but the class is special purpose enough (and tied directly to UI) that I'd rather we just document this limitation and otherwise assume UI thread execution.

The main risk here is that we also need to enforce that NetworkImageView#setImageUrl be called from the main thread. This is because it will initiate a load of the image (if necessary), which will cancel any invalidated in-flight requests. While this wasn't documented, I do think it's clearly the right thing to do, and I think it's worth the risk to enforce that this be called from the UI thread as well.

jpd236 added a commit to jpd236/volley that referenced this issue May 5, 2018
ImageContainer#cancelRequest modifies data structures that are
mutated without a lock on the main thread. Its parent class,
ImageLoader, is documented as requiring that all access happen
on the main thread, so strictly enforce that cancelRequest be
called from the main thread as well. Outside of cancelRequest,
the only uncontrolled caller is NetworkImageView#setImageUrl,
which we also document/enforce as needing to be called from the
main thread.

Add @mainthread annotations and Javadoc clarifications as
appropriate. Clarify that behavior on custom ResponseDeliveries
is undefined.

Fixes google#132
jpd236 added a commit that referenced this issue May 7, 2018
ImageContainer#cancelRequest modifies data structures that are
mutated without a lock on the main thread. Its parent class,
ImageLoader, is documented as requiring that all access happen
on the main thread, so strictly enforce that cancelRequest be
called from the main thread as well. Outside of cancelRequest,
the only uncontrolled caller is NetworkImageView#setImageUrl,
which we also document/enforce as needing to be called from the
main thread.

Add @mainthread annotations and Javadoc clarifications as
appropriate. Clarify that behavior on custom ResponseDeliveries
is undefined.

Fixes #132
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

No branches or pull requests

1 participant