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

Add CoroutineContext configuration option to DiskCache.Builder #2794

Closed
alexvanyo opened this issue Jan 20, 2025 · 3 comments · Fixed by #2797
Closed

Add CoroutineContext configuration option to DiskCache.Builder #2794

alexvanyo opened this issue Jan 20, 2025 · 3 comments · Fixed by #2797
Labels
enhancement New feature or request help wanted Issues that are up for grabs + are good candidates for community PRs

Comments

@alexvanyo
Copy link

I was trying to convert my injectable Dispatchers holder to inject CoroutineContexts, instead of CoroutineDispatchers in the spirit of https://code.cash.app/dispatchers-unconfined.

A case preventing doing this easily is configuring DiskCache, since the DiskCache.Builder only has a cleanupDispatcher configuration option: https://coil-kt.github.io/coil/api/coil-core/coil3.disk/-disk-cache/-builder/index.html#226104852%2FFunctions%2F1772253480

Could a method that accepts a CoroutineContext be added as well?

@alexvanyo alexvanyo added the enhancement New feature or request label Jan 20, 2025
@alexvanyo
Copy link
Author

It looks like the internals do call a CoroutineDispatcher method on the provided dispatcher, so it wouldn't be a super straightforward change if limitedParallelism is necessary:

CoroutineScope(SupervisorJob() + cleanupDispatcher.limitedParallelism(1))

@colinrtwhite
Copy link
Member

Great point! If I remember correctly I think limitedParallelism was used as the DiskLruCache was migrated from OkHttp, which used to use Executors.newSingleThreadExecutor.

I think we could probably refactor the code to remove limitedParallelism as the cleanup code is already guarded by a lock. Instead we could check if a clean up job is already scheduled/in progress and skip launch if it already is. After that we can deprecate DiskCache.Builder.cleanupDispatcher and replace it with DiskCache.Builder.cleanupCoroutineContext. PR welcome!

@colinrtwhite colinrtwhite added the help wanted Issues that are up for grabs + are good candidates for community PRs label Jan 21, 2025
@colinrtwhite
Copy link
Member

colinrtwhite commented Jan 21, 2025

Actually an easier change would be to add DiskCache.Builder.cleanupCoroutineContext then grab the dispatcher from the context with (cleanupCoroutineContext.get(CoroutineDispatcher) ?: Dispatchers.Default).limitedParallelism(1) when creating the cleanupScope.

EDIT: Implemented here: #2797

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Issues that are up for grabs + are good candidates for community PRs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants