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.

Sign up for GitHub

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

Jump to bottom

apollo-datasource-rest caching everything #46

Closed
westphalen opened this issue May 5, 2020 · 3 comments · Fixed by #100
Closed

apollo-datasource-rest caching everything #46

westphalen opened this issue May 5, 2020 · 3 comments · Fixed by #100

Comments

@westphalen
Copy link

apollo-datasource-rest has a HTTPCache that probably works fine, but it is never invoked because RESTDataSource caches the promise itself, the first time it is called (for GET requests).

This seems to defeat the purpose of having HTTPCache altogether, but looking at the Git history, a good deal of work was put into this memoization, so maybe there is a reason for it?

https://github.com/apollographql/apollo-server/blob/f25784d51130bbb9014b727e2f231e5a737f4a50/packages/apollo-datasource-rest/src/RESTDataSource.ts#L270
@frandiox
Copy link

frandiox commented Jul 7, 2020

I guess this is why 408 timeout errors for GET requests are cached 🤔

@hojason117
Copy link

I ran into this issue as well, and the cause turned out to be me not instantiating a new datasource for every request.
More detail can be found in this comment, thought I share it here.

While HTTPCache is responsible for long-term caching based on HTTP semantics and TTL, memoization makes sure we don't perform the same REST call twice within the same GraphQL request.

@Jaxolotl Jaxolotl mentioned this issue Jun 22, 2022
Closed
@Jaxolotl
Copy link

Hi there, I just posted a detailed description of the problem. Hope you find it useful until/if the issue gets fixed

@glasser glasser transferred this issue from apollographql/apollo-server Oct 11, 2022
glasser added a commit that referenced this issue Nov 24, 2022
@glasser
Replace infinite memoization with concurrent deduplication
5a71b46 
RESTDataSource contained a memoization cache (separate from the
HTTP-header-sensitive shared cache) which caches all GET requests
forever. This wasn't even documented for the first four years of
RESTDataSource's existence and led to a lot of confusion.

In this new major version, this cache will instead by default only
de-duplicate *concurrent* GET requests.

This also introduces `deduplicationPolicyFor()` which lets you tune how
de-duplication works, including restoring v4 semantics (see the
changeset for code snippets).

Fixes #40.
Fixes #72.
Fixes #73.
Fixes #46.

Maybe fi-xes #39 but we could use a unit test.

Somewhat addresses #65 but only for the de-duplication cache (we should
still allow direct control over the cache key for the HTTP).
@glasser glasser mentioned this issue Nov 24, 2022
Merged
glasser added a commit that referenced this issue Nov 28, 2022
@glasser
Replace infinite memoization with concurrent deduplication
0f286a8 
RESTDataSource contained a memoization cache (separate from the
HTTP-header-sensitive shared cache) which caches all GET requests
forever. This wasn't even documented for the first four years of
RESTDataSource's existence and led to a lot of confusion.

In this new major version, this cache will instead by default only
de-duplicate *concurrent* GET requests.

This also introduces `deduplicationPolicyFor()` which lets you tune how
de-duplication works, including restoring v4 semantics (see the
changeset for code snippets).

Fixes #40.
Fixes #72.
Fixes #73.
Fixes #46.

Maybe fi-xes #39 but we could use a unit test.

Somewhat addresses #65 but only for the de-duplication cache (we should
still allow direct control over the cache key for the HTTP).
glasser added a commit that referenced this issue Nov 28, 2022
@glasser
Replace infinite memoization with concurrent deduplication
2392572 
RESTDataSource contained a memoization cache (separate from the
HTTP-header-sensitive shared cache) which caches all GET requests
forever. This wasn't even documented for the first four years of
RESTDataSource's existence and led to a lot of confusion.

In this new major version, this cache will instead by default only
de-duplicate *concurrent* GET requests.

This also introduces `deduplicationPolicyFor()` which lets you tune how
de-duplication works, including restoring v4 semantics (see the
changeset for code snippets).

Fixes #40.
Fixes #72.
Fixes #73.
Fixes #46.
Fixes #39 (though it "introduces" #102).

Somewhat addresses #65 but only for the de-duplication cache (we should
still allow direct control over the cache key for the HTTP).
glasser added a commit that referenced this issue Nov 29, 2022
@glasser
Replace infinite memoization with concurrent deduplication
1db9945 
RESTDataSource contained a memoization cache (separate from the
HTTP-header-sensitive shared cache) which caches all GET requests
forever. This wasn't even documented for the first four years of
RESTDataSource's existence and led to a lot of confusion.

In this new major version, this cache will instead by default only
de-duplicate *concurrent* GET requests.

This also introduces `deduplicationPolicyFor()` which lets you tune how
de-duplication works, including restoring v4 semantics (see the
changeset for code snippets).

Fixes #40.
Fixes #72.
Fixes #73.
Fixes #46.
Fixes #39 (though it "introduces" #102).

Somewhat addresses #65 but only for the de-duplication cache (we should
still allow direct control over the cache key for the HTTP).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Replace infinite memoization with concurrent deduplication apollographql/datasource-rest
4 participants
@frandiox @Jaxolotl @westphalen @hojason117