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

Bug: axios.waiting grows when the max entries is set + etag handling #833

Open
bukowskiadam opened this issue May 10, 2024 · 3 comments
Open
Assignees
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed

Comments

@bukowskiadam
Copy link

What happened?

Hey Arthur,

Recently we've migrated our code to axios and we've used your package to handle caching. Thank you for your work here ❤️
After the migration, we've noticed that memory consumption of our app presents a "lovely" toothsaw pattern 😬 I've spent quite a lot of time trying to figure out what's wrong.

Here is the recap - sorry for it being long.

Etag / last-modified

This is not a bug, but that caused us to store many entries forever.

Every resource with an etag header (or last-modified) in the response (the default express app adds it), is kept forever, and there is no simple way to disable it (or I haven't found one).

I've mitigated it with the following code that removes those headers and assign default stale ttl:

import axios from 'axios';
import {
  setupCache,
  buildMemoryStorage,
  defaultHeaderInterpreter,
} from 'axios-cache-interceptor';

const DEFAULT_TTL_MS = 5 * 60 * 1000;
const DEFAULT_STALE_MS = 60 * 1000;

const CLONE_DATA = false;
const CLEANUP_INTERVAL_MS = 5 * 60 * 1000;
// set to unlimited to avoid the hanging promise in the `waiting` object
const MAX_ENTRIES = undefined;

const axiosCache = setupCache(axiosCacheInstance, {
  ttl: DEFAULT_TTL_MS,
  storage: buildMemoryStorage(CLONE_DATA, CLEANUP_INTERVAL_MS, MAX_ENTRIES),
  headerInterpreter: (headers) => {
    // remove unwanted headers that force the cache entry to be stalled/cached forever
    headers && delete headers.etag;
    headers && delete headers['last-modified'];

    let ret = defaultHeaderInterpreter(headers);

    if (typeof ret === 'object' && 'cache' in ret) {
      // we have the cache and stale time or possibly null. make sure it's our default value
      ret.stale = DEFAULT_STALE_MS;
    } else if (typeof ret === 'number') {
      // we have the ttl number, but we have to add the stale time
      ret = {
        cache: ret,
        stale: DEFAULT_STALE_MS,
      };
    }

    if (ret === 'not enough headers') {
      // by default it would use the DEFAULT_TTL_MS, but `stale` would be `undefined`
      // in order to override `stale` we need to return an object
      ret = {
        cache: DEFAULT_TTL_MS,
        stale: DEFAULT_STALE_MS,
      };
    }

    return ret;
  },
});

waiting

To mitigate the problem with stalled entries kept forever, we've tried using MAX_ENTRIES in the memory storage, but that didn't improve the situation but the problem was different.

Where we have many parallel requests fired, and we exceed the max entries limit, the memory adapter removes some of them. But they are kept in the axios.waiting object. After the request is resolved they are not removed from there. Probably until you try to hit the same request.id again (the same url, method, query).

I believe the problem comes from this part of the code:

const cache = await axios.storage.get(response.id, config);
if (
// If the request interceptor had a problem or it wasn't cached
cache.state !== 'loading'
) {
if (__ACI_DEV__) {
axios.debug({
id: response.id,
msg: "Response not cached and storage isn't loading",
data: { cache, response }
});
}
return response;
}

Because when you're removing the entry from the storage and it's not there, the returned value is

{ state: 'empty' }

so this if is true, so we end the response interceptor, and we do not remove / resolve other waiting requests.

In our very specific case, the URL passed to Axios was sliced from a very long document, which caused Node to keep the reference to the original string. (slice / substring / regex match, does not create the copy of a string, but points to the fragment of the original string).

proof of concept

Here is my repo with test cases describing the two issues we had:
https://github.com/bukowskiadam/axios-cache-interceptor-memory-issue

I hope it gives some working example so you can validate the problems.

What's next

(In other words: what is this issue about)

  1. Fixing axios.waiting memory leak
  2. Maybe provide an easy way like etag: false to disable handling ETag headers.

Best regards,
Adam

axios-cache-interceptor version

v1.5.2

Node / Browser Version

Node v20.12.2

Axios Version

v1.6.8

What storage is being used

Memory Storage

Relevant debugging log output

Output too long - see the linked project
@bukowskiadam bukowskiadam added bug Something isn't working triage Needs triage labels May 10, 2024
@arthurfiorette
Copy link
Owner

@bukowskiadam
Copy link
Author

Hi, as you described in the docs (and I checked the code), the only valid options are true | string. false does nothing.
I've checked that here: https://github.com/bukowskiadam/axios-cache-interceptor-memory-issue/blob/e7286464f8aa036fc6eace6c35baee89824dd6b0/test/etag.js#L29

Imo this code:

if (cacheConfig.etag && cacheConfig.etag !== true) {
response.headers[Header.XAxiosCacheEtag] = cacheConfig.etag;
}
allows only to override etag, but not to delete it 🤷🏻

@arthurfiorette
Copy link
Owner

Hey @bukowskiadam, you're right!

I do not have enough spare time to work in this project right now... Anyone up to a PR?

@arthurfiorette arthurfiorette added help wanted Extra attention is needed good first issue Good for newcomers and removed triage Needs triage labels May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants