Implement InFlightRequestStrategy and DeDupeInFlightRequestStrategy #2995

luciofm · 2025-05-22T00:13:34Z

This fixes #1461, to avoid multiple parallel network requests for the same URL.

I've basically created a simple InFlightRequestStrategy interface, with a DEFAULT implementation that does nothing.

And a concrete implementation DeDupeInFlightRequestStrategy, that check for a current in-flight request for the same url and waits for it.

Then on NetworkFetcher I've simply wrapped the fetch() method with InFlightRequestStrategy.apply().

With the DEFAULT implementation, it will just invoke the lambda (call doFetch()). And using the DeDupe strategy, it will check for in-flight requests and wait on it.

Note: I haven't update the .api files yet...

revonateB0T · 2025-05-22T02:54:51Z

coil-network-core/src/commonMain/kotlin/coil3/network/InFlightRequestStrategy.kt

+        block: (suspend () -> FetchResult),
+    ): FetchResult {
+        var shouldWait = false
+        val request = mutex.withLock {


I suggest to use non-suspend mutex here as critical region is small & non-suspend.

Fair enough.

revonateB0T · 2025-05-22T02:59:59Z

coil-network-core/src/commonMain/kotlin/coil3/network/InFlightRequestStrategy.kt

+
+        return request.use {
+            block()
+        }.also {


Given we launch 2 same requests A, B. A is active, B is waiting for A.
What about A is cancelled? we doesn't remove it from inFlightRequests here.
Moreover, now that B is not cancelled, should we cancel & relaunch network request here? it does harm performance.

If A is cancelled, the request will be removed from the inFlightRequests map and B will proceed...

If there are more requests waiting, on this particular case, all of them will trigger a network request.

If A is cancelled, the request will be removed from the inFlightRequests map and B will proceed...

If there are more requests waiting, on this particular case, all of them will trigger a network request.

But we did't remove it, the also block is not getting executed due to CancellationException, it's not wrapped by FetchResult. You should use try finally instead.
Moreover, the suspend Mutex will never work in cancelled CoroutineContext unless you wrap it with withContext(NonCancellable)

If the request is canceled, the Deferrable will be completed, and will not block other requests form going through, but I'll move to a try/finally to remove the request from the map.

If the request is canceled, the Deferrable will be completed, and will not block other requests form going through, but I'll move to a try/finally to remove the request from the map.

I think we should spread the exception(FetchResult) to waiting request when request is failed with network error to avoid meaningless requests.
And resume only one waiting request when inflighting request is cancelled.

I wanted to make it really simple, and as less invasive as possible on the current code/functionality while solving the most common use cases...

Lets see what @colinrtwhite has to say about this

I wanted to make it really simple, and as less invasive as possible on the current code/functionality while solving the most common use cases...

Lets see what @colinrtwhite has to say about this

He's on vacation #2959 (comment)

Maybe we could have 2 implementations, the current one proposed on the PR renamed as SimpleInFlightRequestStrategy, a simple strategy that handles the most common cases:

The first network request succeeds and all other waiters are unblocked to hit the disk cache in parallel

All queued network requests for the same URL would fail. We unblock all waiting requests to fail in parallel.

And have a second, more complex, implementation

One request is allowed to go through, all others are waiting on a Channel<Unit>

The request succeeds:

The channel is closed, and all waiting coroutines are allowed to run in parallel, hitting the disk cache.

The request fails:

Tries to send on the channel, if fails (no one waiting), close the channel and remove the InFlightRequest from the map

If send is successful, this means another coroutine will wake up and run the request, this new coroutine will now be the channel owner/controller and responsible for the cleanup. Rinse and repeat.

https://github.com/coil-kt/coil/pull/2995/files#diff-d957f14d6fa8fb2c0110fd58003e7d65e4550a2cd3f2fb7bf2b2de8eb2fa234eR88-R178

revonateB0T · 2025-05-22T03:07:52Z

I suggest to look into our MergeInteceptor https://github.com/FooIbar/EhViewer/blob/1548e16fbe7250785d5d13e55615131777ac11b6/app/src/main/kotlin/com/hippo/ehviewer/coil/MergeInterceptor.kt#L34

It handles same-url-request debounce well using kotlin flow.
Given request order:
Launch A -> Launch B -> Cancel A
where A, B use same URL, launched network request is executed as is.

Given request order:
Launch A -> Cancel A -> Launch B
where A, B use same URL, delay 100ms on each operation, launched network request is executed as is, we avoid relaunch request with a 100ms cancel-delay trade-off.

colinrtwhite

@luciofm Thanks for working on this! I left a few comments, but I think these are the main things we should focus on/think about:

InFlightRequestStrategy.apply currently couples us to the disk cache key. We might not be able to change this especially if block must capture the disk cache reads as well (instead of only the network request).
InFlightRequestStrategy.apply block captures Option, which has a reference to the locally-scope Context. This could leak a Fragment/Activity if apply is cancelled, but block isn't.
I think we probably only want to ship 2 request strategies - not 3: InFlightRequestStrategy.DEFAULT (i.e. no deduping) and ideally a version of deduping that uses refcounting and only executes the network request exactly once (instead of passing the baton on a failure).
We'll need to preserve the existing functions/constructors that don't have InFlightRequestStrategy arguments. This is to avoid breaking binary compatibility.

EDIT: I just realized why we can't do refcounting with the current strategy, as we need to do separate disk cache reads for each request after the network request finishes. Going to think about this a bit... The main thing I'd like to solve for is allowing a network request to keep executing even if NetworkFetcher.fetch is cancelled, but there are still non-cancelled observers.

colinrtwhite · 2025-05-29T22:58:58Z

coil-network-core/src/commonMain/kotlin/coil3/network/NetworkFetcher.kt

    class Factory(
        networkClient: () -> NetworkClient,
        cacheStrategy: () -> CacheStrategy = { CacheStrategy.DEFAULT },
+        inFlightRequestStrategy: () -> InFlightRequestStrategy = { InFlightRequestStrategy.DEFAULT },


This breaks binary compatibility. We'll need to keep old constructors similar to here.

colinrtwhite · 2025-05-29T23:00:05Z

coil-network-core/src/commonTest/kotlin/coil3/network/NetworkFetcherTest.kt

            diskCache = lazyOf(null),
            cacheStrategy = lazyOf(CacheStrategy.DEFAULT),
            connectivityChecker = ConnectivityChecker(context),
+            inFlightRequestStrategy = lazyOf(InFlightRequestStrategy.DEFAULT)


Nit: this is ordered after connectivityChecker here, but before connectivityChecker in the constructor

colinrtwhite · 2025-05-29T23:01:26Z

coil-network-ktor2/src/commonMain/kotlin/coil3/network/ktor2/KtorNetworkFetcher.kt

 @JvmName("factory")
 fun KtorNetworkFetcherFactory(
    httpClient: HttpClient,
+    inFlightRequestStrategy: InFlightRequestStrategy = InFlightRequestStrategy.DEFAULT


Let's create new functions that include inFlightRequestStrategy and mark the old as deprecated hidden similar to here. We need to do this to preserve binary compatibility.

colinrtwhite · 2025-05-29T23:15:06Z

coil-network-core/src/commonMain/kotlin/coil3/network/InFlightRequestStrategy.kt

+import okio.Closeable
+import okio.use
+
+interface InFlightRequestStrategy {


Could you add a short doc description to this and mark it as @ExperimentalCoilApi? Thanks

colinrtwhite · 2025-05-29T23:15:51Z

coil-network-core/src/commonMain/kotlin/coil3/network/InFlightRequestStrategy.kt

+
+interface InFlightRequestStrategy {
+    suspend fun apply(key: String, block: (suspend () -> FetchResult)): FetchResult {
+        return block()


Let's remove the default implementation and move it to DEFAULT.

colinrtwhite · 2025-05-29T23:33:19Z

coil-network-core/src/commonMain/kotlin/coil3/network/InFlightRequestStrategy.kt

+import okio.use
+
+interface InFlightRequestStrategy {
+    suspend fun apply(key: String, block: (suspend () -> FetchResult)): FetchResult {


This couples us to key (the disk cache key), which I was hoping to avoid as it prevents custom implementations where a user can specify a custom "keyer". For example, I'm thinking of a case where a user might want a request's headers to be considered part of the cache key. It'll be possible with the current implementation, but they'll have to set ImageRequest.diskCacheKey manually. We could introduce a diskCacheKeyFactory, though I think that'll introduce a lot of other complexities.

Also, I think block can leak memory depending on the implementation. It'll implicitly hold a reference to Options, which will hold the local Context for that request. If apply is cancelled, but we keep executing block then we would leak the underlying Context. I think we need to figure out a way to avoid capturing the Options and ideally guard against accidentally capturing it in the future.

For now I think we could add a this limitation to the InFlightRequestStrategy documentation, but creating a diskCacheKeyFactory makes sense for these cases...

About the leaking Context, I've added a new safeDiskCacheKey that copies options.diskCacheKey if available, or just use the url.

colinrtwhite · 2025-05-29T23:41:02Z

coil-network-core/src/commonMain/kotlin/coil3/network/InFlightRequestStrategy.kt

+        }
+    }
+
+    data class InFlightRequest(


Suggested change

data class InFlightRequest(

private class InFlightRequest(

Removed data to save a few bytes and minimize the public API.

colinrtwhite · 2025-05-30T00:08:54Z

coil-network-core/src/commonMain/kotlin/coil3/network/InFlightRequestStrategy.kt

+        try {
+            return block()
+        } catch (ex: Exception) {
+            val successfullyPassedBaton = channel.trySend(Unit).isSuccess


Will this launch a new request if the first request is cancelled and it falls back to a second observer? I'm wondering if we can create an implementation that refcounts the observers and only cancels the request if the refcount drops to 0.

This works like a serial queue until block() succeeds/returns.
If block throws - either cancelation or it fails (404 for example), it will awake exactly one waiter to try again.
If block succeeds, it will close the channel, releasing all waiters to proceed.

luciofm · 2025-05-31T21:21:13Z

I think I've fixed all/most your comments, specially the binary compatibility ones.

I think we probably only want to ship 2 request strategies - not 3: InFlightRequestStrategy.DEFAULT (i.e. no deduping) and ideally a version of deduping that uses refcounting and only executes the network request exactly once (instead of passing the baton on a failure).
Agree with having just a DEFAULT that does nothing and one implementation... I've moved the SimpleInFlightRequestStrategy as a sample on InFlightRequestStrategy doc.

I've added a new implementation for discussion (we should keep just one in the end). This one has an optional delay in removing the request key from the inFlightRequests map, to avoid creating new channels on scrolling for example. If you don't think this is a good idea, we can just remove the delay, but I think this implementation is cleaner than the first one... It works basically the same.

Implement InFlightRequestStrategy and DeDupeInFlightRequestStrategy

48ffa53

revonateB0T reviewed May 22, 2025

View reviewed changes

luciofm added 6 commits May 22, 2025 12:41

Use synchronized and try/finally

9a0f302

Add another implementation...

1cb899f

Set just cleanup flag

3aa1d49

Some cleanup

df2e22e

.api updates and cleanup

3cfd8dc

Update APIs

51cd871

colinrtwhite reviewed May 30, 2025

View reviewed changes

luciofm added 4 commits May 30, 2025 19:42

Keep old constructors

d9a7bee

Keep old factory constructor. safeDiskCacheKey

762bb56

Removed SimpleInFlightRequestStrategy. Added yet another for discussion.

b7bdcb8

Update APIs

9c41556

Remove a local test file accidently added to the api files

5a00a71

elyesmansour mentioned this pull request Jun 17, 2025

Avoid multiple parallel network requests for the same URL #1461

Open

tak8997 mentioned this pull request Nov 12, 2025

Prevent multiple parallel network requests for the same URL #3228

Closed

Implement InFlightRequestStrategy and DeDupeInFlightRequestStrategy #2995

Are you sure you want to change the base?

Implement InFlightRequestStrategy and DeDupeInFlightRequestStrategy #2995

Uh oh!

Conversation

luciofm commented May 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

revonateB0T May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

revonateB0T May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luciofm May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

revonateB0T commented May 22, 2025

Uh oh!

colinrtwhite left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luciofm commented May 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

revonateB0T May 22, 2025 •

edited

Loading

revonateB0T May 22, 2025 •

edited

Loading

luciofm May 26, 2025 •

edited

Loading

colinrtwhite left a comment •

edited

Loading