Prevent multiple parallel network requests for the same URL #3228
Conversation
| private fun generateRequestKey(url: String, options: Options): String { | ||
| return buildString { | ||
| append(url) | ||
| append('|') | ||
| append(options.httpMethod) | ||
| append('|') | ||
| append(options.httpBody?.hashCode() ?: 0) | ||
| append('|') | ||
| val headers = options.httpHeaders.asMap() | ||
| .flatMap { (name, values) -> values.map { value -> name to value } } | ||
| .sortedBy { it.first } | ||
| .joinToString(separator = ",") { "${it.first}:${it.second}" } | ||
| append(headers) | ||
| append('|') | ||
| append(options.networkCachePolicy) | ||
| append('|') | ||
| append(options.diskCachePolicy) | ||
| append('|') | ||
| append(options.diskCacheKey) | ||
| } | ||
| } |
There was a problem hiding this comment.
I would appreciate some advice on how to make a request key
There was a problem hiding this comment.
Ideally, I think we want to make it configurable. We should make it an interface argument to NetworkFetcher so users can provide their own. By default I think our request key should only be url.
|
@colinrtwhite Could you please check this PR? |
| } | ||
| } | ||
| if (deferred !== newDeferred) { | ||
| return deferred.await() |
There was a problem hiding this comment.
This will propagate cancellation so if the request that started first is cancelled, all observers will also be cancelled. Ideally if we have >= 1 requests still awaiting and not cancelled, we shouldn't cancel the underlying network operation and let it complete.
| return deferred.await() | ||
| } | ||
|
|
||
| var snapshot = readFromDiskCache() |
There was a problem hiding this comment.
Let's move this and everything below it into a separate method. That way we can call that method and wrap it with deferred.complete etc. vs. completing the deferred before each return.
| private val diskCache: Lazy<DiskCache?>, | ||
| private val cacheStrategy: Lazy<CacheStrategy>, | ||
| private val connectivityChecker: ConnectivityChecker, | ||
| private val pendingRequests: HashMap<String, CompletableDeferred<SourceFetchResult>>, |
There was a problem hiding this comment.
We should probably refactor pendingRequests + the mutex into a separate class that hides these as an implementation detail. It would be similar to how DeDupeInFlightRequestStrategy has both a non-synchronized implementation and a synchronized implementation in this PR: #2995
|
|
||
| public final class coil3/network/NetworkFetcher : coil3/fetch/Fetcher { | ||
| public fun <init> (Ljava/lang/String;Lcoil3/request/Options;Lkotlin/Lazy;Lkotlin/Lazy;Lkotlin/Lazy;Lcoil3/network/ConnectivityChecker;)V | ||
| public fun <init> (Ljava/lang/String;Lcoil3/request/Options;Lkotlin/Lazy;Lkotlin/Lazy;Lkotlin/Lazy;Lcoil3/network/ConnectivityChecker;Ljava/util/HashMap;Lkotlinx/coroutines/sync/Mutex;)V |
There was a problem hiding this comment.
Adding constructor arguments to NetworkFetcher breaks binary compatibility so we'll have to keep the old constructors as secondary constructors and add @Deprecated("Kept for binary compatibility.", level = DeprecationLevel.HIDDEN) to them.
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…gin to v1.9.3 (coil-kt#3221) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
coil-kt#3225) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…in to v0.35.0 (coil-kt#3227) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
… v0.8.2 (coil-kt#3243) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* chore(deps): update plugin screenshot to v0.0.1-alpha12 * Use JDK 21. * Suppress warning. --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Colin White <colin@colinwhite.me>
…v1.4.2 (coil-kt#3255) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
coil-kt#3258) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* fix(deps): update kotlin monorepo to v2.3.0 * Fix build issues. * Fix warning. * Fix warning. * Update Poko. --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Colin White <colin@colinwhite.me>
…t. (coil-kt#3170) * Migrate to the Kotlin plugin's binary compatibility validation support. # Conflicts: # AGENTS.md # gradle/libs.versions.toml * Update API.
* Remove the Okio polyfill JS workaround. It only removes a warning on certain variants and one of the APIs it relies on is deprecated in Kotlin 2.3.0. # Conflicts: # build.gradle.kts * Update yarn.lock * Update yarn.lock
4 participants
Summary
This PR introduces a mechanism to prevent multiple parallel network requests for the same URL in Coil.
Details
HashMapandCompletableDeferredto track in-flight requests.Motivation