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

Support thread interruption in cancel #378

Closed
zhanghai opened this issue Apr 30, 2020 · 3 comments · Fixed by #395 or #414
Closed

Support thread interruption in cancel #378

zhanghai opened this issue Apr 30, 2020 · 3 comments · Fixed by #395 or #414
Labels
enhancement New feature or request

Comments

@zhanghai
Copy link
Contributor

Is your feature request related to a problem? Please describe.
I'm working on a file manager with fast scroll support. In a directory with hundreds of images, if the user fast scrolls the list of files, the thumbnail loading isn't as smooth as what Glide used to provide. I think this is because Coil's decoders (mainly BitmapFactoryDecoder) isn't getting interrupted upon cancel, which I heard Glide does (via Future.cancel(true)).

Describe the solution you'd like
Coil decoders should interrupt its thread upon job cancellation. An implementation of this has been proposed at Kotlin/kotlinx.coroutines#1947, however I'm not sure when it will land, so maybe we should also consider rolling our own since this is important for smooth image loading.

Additional context
I didn't find any usage of Future or interrupt in this project, and I also looked at BitmapFactoryDecoder where I didn't find anything special, so I'm assuming that the current Coil job cancellation is just a coroutine cancenllation. Sorry in advance if I overlooked anything, because I'm not a coroutine expert yet.

@zhanghai zhanghai added the enhancement New feature or request label Apr 30, 2020
@colinrtwhite
Copy link
Member

colinrtwhite commented Apr 30, 2020

Interesting! I think this is a great idea. You're right that Coil currently only uses existing coroutines mechanisms (mostly suspendCancellableCoroutine and ensureActive) to handle cancellation, which can cause the request to go on slightly longer than required.

Looking at the runInterruptible implementation, I'm not sure how easy it would be lift into Coil, however if you have a good idea of how to implement it I'm open to a PR. Otherwise, I'm leaning towards waiting for the next Coroutines library release. Considering they just merged the PR hopefully it'll land soon.

@zhanghai
Copy link
Contributor Author

KotlinX Coroutines 1.3.6 has been released with runInterruptible. I experimented with wrapping IO calls in BitmapFactoryDecoder, and there seems to be some small improvements when fast scrolling, but I don't have any benchmarking for this so I'm not so sure yet.

@colinrtwhite
Copy link
Member

colinrtwhite commented May 13, 2020

Reopening this as runInterruptible was causing issues with caching on master.

The issue is we can't interrupt the thread while it's reading from an OkHttp DiskLruCache's BufferedSource. Doing so sets hasJournalErrors to true, which causes all subsequent requests to miss the cache + not be saved to the cache until the next journal rebuild.

I think we can still support thread interruption during decoding by using a ForwardingSource and deferring thread interruption if we're in ForwardingSource.read, however I'll need to test that out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants