-
Notifications
You must be signed in to change notification settings - Fork 2k
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
DataSources: InMemory LRU Cache has Infinite maxSize #2252
Comments
You're right that it's currently not evicting anything. In terms of the second half of your I'm curious what your thoughts are in terms of a reasonable |
I am not sure I understand the default size calculation. function defaultLengthCalculation(item: any) {
if (Array.isArray(item) || typeof item === 'string') {
return item.length;
}
// Go with the lru-cache default "naive" size, in lieu anything better:
// https://github.com/isaacs/node-lru-cache/blob/a71be6cd/index.js#L17
return 1;
} According to this function function defaultLengthCalculation(item: any) {
if (typeof item === 'object') {
return Buffer.byteLength(JSON.stringify(item));
}
if (typeof item === 'string') {
return Buffer.byteLength(item);
}
return 1;
} For I don't think a max size should be set for each DataSource. Isn't it just one cache shared between different DataSources? |
I think this is the expected behavior for a generic LRU which is meant to be utilized in many places — not just To be clear: The
There are multiple caches, one for each data source (see apollo-server/packages/apollo-datasource-rest/src/RESTDataSource.ts Lines 51 to 55 in d1f471d
This all to say, I think we should consider passing I'm curious what @martijnwalraven thinks. |
Hi, I would to like add one more thing related to this. I am using apollo-rest-datasource to fetch data from a REST endpoint. I faced the same issue that requests are cached infinitly, I would like to add that even if the headers changes still we get the same response. For example, if we get a list of data from an endpoint it is cache, this request had authorization header set to ABCD. Now, when we request the same endpoint with authorization header WXYZ we still get the same response. It doesn't even identify the change, it only checks if the endpoint is same. We use authorization headers for access related details, but we are not able to do it. What ever access a the person first time had is cached for everyone else. Please let me know how to handle this Thanks |
@ThiruvenkatamR It looks like the caching mechanism for apollo-server/packages/apollo-datasource-rest/src/RESTDataSource.ts Lines 58 to 65 in d1f471d
I am not convinced that the following statement is accurate
You may be able to solve your problem by implementing your own protected cacheKeyFor(request: Request): string {
if(request.headers.get('authorization'))
return `${request.headers.get('authorization')}|${request.url}`;
return request.url
} |
Thanks @nharlow89 Will check it out. Much appreciated |
Just to make sure, to create a LRU cache for 100 items, I have to do this, right? new ApolloServer({
cache: new InMemoryLRUCache({
maxSize: 100,
sizeCalculator: () => 1,
}),
}); It's a bit unintuitive that I have to provide the size calculator but not providing would probably eat all my cache with possibly a single string, right? |
Would a default of 0MB coupled with an explanation in the docs on how to enable the cache be more reasonable than an unbounded cache? cries in out of memory exception |
The class in question has moved from |
From DataSources docs.
This is misleading considering that the default cache has a maxSize of
Infinite
. You can have any eviction policy, but if the cache size is infinite, you'll never evict any keys.My suggestion is to either:
a) Update the docs saying that by default the cache has no memory limit
b) Set a reasonable maxSize, and update the function that calculates size of items, since currently it doesn't really make sense (length of item if it's a string or array, length of stringified object, or 1 in other cases).
I think the second option is the cleanest solution, and if you are okay with it I'll make a PR for it.
The text was updated successfully, but these errors were encountered: