Skip to content

Change MemoryStore to function like an LRU to prevent memory leaks #128

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

AlexJuarez
Copy link

Added LRU logic to the memoryStore. It makes for a sane default if deploying a micro service.

@dougwilson
Copy link
Contributor

This is too complicated and we don't want to maintain any session store, period. The memory store only exists for legacy reasons, and it does delete expired sessions, so they don't last forever (plus, your store is just as useless in production as our store: anyone can DoS your site by generated more sessions than the limit, kicking out legitimate users).

Please feel free to maintain this and publish to npm. We'd be happy to link to your module.

@dougwilson
Copy link
Contributor

I'm going to actually keep this open for when I get to a computer and can look more at the implementation, but I may still end up closing it if it's meant to be something that would actually function in production.

@dougwilson dougwilson reopened this Mar 1, 2015
@AlexJuarez
Copy link
Author

Thanks, I think in the current version of the store expired sessions are only removed when they are read through all, get, and length (since it calls all). But if expired sessions are not read again, it does not appear that they are removed from memory.

Also I think that keeping track of the length, might provide an easy performance bonus over iterating through all of the session objects. (albeit marginal)

@dougwilson
Copy link
Contributor

Sure, but the built-in MemoryStore is (hopefully) supposed to be crappy to discourage people from ever using it apart from simplifying development or debugging.

@AlexJuarez
Copy link
Author

@dougwilson Where did you land on this change? Want more test cases?

@dougwilson
Copy link
Contributor

I still think it's better just to release it as it's own module.

@dougwilson
Copy link
Contributor

Do you think we can leverage a module like https://github.com/isaacs/node-lru-cache instead of re-implementing (and maintaining) a LRU cache in here? That should provide what you're looking for without us need to actually maintain a LRU implementation.

@AlexJuarez
Copy link
Author

I guess so, his implementation is bit different then the one I proposed. It uses a buffer to keep track of items in the cache. But it wouldn't really matter, it does the same basic thing, which is setting a limit on the number of session objects.

@dougwilson
Copy link
Contributor

Gotcha. If you know of a good MRU/LRU module to use, I think that would convince me here, since we would be maintaining a caching mechanism (and we don't even want to maintain site in here anyway; the memory store is just to get up and running).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants