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

Memory leak in the implementation of RequestWithCache. #66

Closed
happy-san opened this issue Jul 30, 2021 · 2 comments · Fixed by #76
Closed

Memory leak in the implementation of RequestWithCache. #66

happy-san opened this issue Jul 30, 2021 · 2 comments · Fixed by #76
Assignees
Labels
hacktoberfest help wanted Extra attention is needed

Comments

@happy-san
Copy link

Description

In the current implementation, a cache entry is only deleted when it is requested and it has expired. If the cache entry is never requested, it would never be deleted. Combined with the fact _responseCache has no limit on how many entries it can store, we've got a leak.

    if (cacheEntry) {
      if (Date.now() - cacheEntry.requestTimestamp < cacheResponseForSeconds * 1000) {
        // Cache entry is still valid, return it
        return Promise.resolve(cacheEntry.response)
      } else {
        // Cache entry has expired, so delete it explicitly
        delete this._responseCache[requestFunctionArgumentsJSON]
      }
    }

Steps to reproduce

  1. Enable caching of results
  2. Keep searching for stuff without ending the session, memory usage would steadily increase.

Expected Behavior

There should be an eviction policy in place to get rid of the unwanted cached entries and preferably an upper limit on how many results can be cached. I'd recommend implementing Least Recently Used algorithm.

@dcantu476
Copy link
Contributor

@jasonbosco @happy-san Is this still a concern?

@jasonbosco
Copy link
Member

@dcantu476 Yes it is. Mainly a concern if users search say several 100 times without a page refresh in a single session.

Could happen more often in a React Native environment for example.

Definitely want to address it. Do you want take a stab at it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants