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

Fix item wrongly removed from http cache when error in subscriptions #4537

Merged
merged 5 commits into from
Nov 29, 2022

Conversation

BoD
Copy link
Contributor

@BoD BoD commented Nov 28, 2022

Issue encountered here.

For the http cache, we add 2 interceptors:

  1. an HttpInterceptor that deals with caching
  2. an ApolloInterceptor which cleans up the cache if there was an error

The cache key is computed in 1. and used in 2. But 1. is executed for queries and mutations only, whereas 2. is also executed for subscriptions. When a subscription resulted in an error, an arbitrary (the most recent one) cache key would be incorrectly cleared from the cache. The proposed fix is to do this cleanup only for queries and mutations.

@netlify
Copy link

netlify bot commented Nov 28, 2022

Deploy Preview for apollo-android-docs canceled.

Name Link
🔨 Latest commit c73336e
🔍 Latest deploy log https://app.netlify.com/sites/apollo-android-docs/deploys/6385c2cc8361e30009fdc630

@martinbonnin
Copy link
Contributor

The root problem seems to be that cacheKey is global to an ApolloClient and not a request. There is still a bug if two request run concurrently. They might revert the wrong cache key.

@BoD
Copy link
Contributor Author

BoD commented Nov 28, 2022

Good point! Hmm, we could store the cacheKey as a header on the http response, which can be retrieved inside the .onEach { part, but not sure how to deal with the catch { part 🤔

@martinbonnin
Copy link
Contributor

not sure how to deal with the catch { part 🤔

Mmmmm right. The issue is that the cache key is computed at the HTTP layer but we're reading it at the GraphQL layer.

You could maintain a map GraphQL request uuid : HTTP cache key in the HTTP interceptor, just after computing it. Then you can read this map in the catch {} block.

@@ -72,16 +73,19 @@ fun ApolloClient.Builder.httpCache(
directory = directory,
maxSize = maxSize,
)
var cacheKey: String? = null
val apolloRequestToCacheKey = mutableMapOf<String, String>()
var apolloRequestUuid: Uuid? = null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still racy. This code can be accessed from multiple threads. requestUuid needs to be passed down to the interceptor and apolloRequestToCacheKey protected by a lock

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right of course! 👍 A fix in 152e746.

@BoD BoD merged commit f0348a8 into main Nov 29, 2022
@BoD BoD deleted the fix-http-cache-subscriptions branch November 29, 2022 09:28
@sam43
Copy link

sam43 commented Dec 5, 2022

Hi @BoD,
Hope you are doing well, and great work guys. I have tested the v3.7.2-SNAPSHOT version, by creating the .jar version. It works perfectly fine, I will be waiting for the next release for the library to integrate into my project which's release is kinda blocked right now! But no worries at all. Thanks for your support, regards.

@BoD
Copy link
Contributor Author

BoD commented Dec 5, 2022

Hi @sam43, thanks a lot for the feedback, glad that the fix works for you! We've just released v3.7.2 which includes the fix. Thanks again for reporting the issue 🙏

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

Successfully merging this pull request may close these issues.

3 participants