Implemented a full featured MemoryStore without leaks

[danielgindi]

The current implementation lacks a lot. It does not do a TTL for sessions that have no cookie expiry, and it does not clean up expired sessions so they can leak endlessly.

Of course in express' documentation a MemoryStore is defined as something that should not be used, but there are cases where Redis is an overkill, and you do want a MemoryStore on a single process.

For those cases, I have re-implemented the memory store in a more stable manner, with TTL and with automatic cleanup after expiration.

[JamesMGreene]

👍 😄

[gabeio]

You might want to look at the replies to a similar pr (#128) as @dougwilson mentioned:

the memory store is just to get [the site] up and running

[dougwilson]

Hi everyone, I haven't close this pull request, because there is some possible benefit I can see, but in general, if you haven't already, I recommend publishing a memory store to npm and we can 100% add it to the list of stores!

[danielgindi]

I'll wait for a decision whether this is going to be merged or not.
The thing is express does come with a MemoryStore, and as much as you'd like people not to use it- they actually do. So why not just fix its shortcomings?

There are many cases where we would like developers to do things differently, but they vote with their code. They do it despite our likes and dislikes. It's like the discussions about circular imports in ES6 imports- they ended up being in the specs because people rely on them.

[joewagner]

For the sake of voting, I say nay... It seems a lot like code bloat.
The current store handles debugging and running simple tests. What is the benefit of maintaining a more complicated store here?

[gabeio]

The thing is express does come with a MemoryStore, and as much as you'd like people not to use it- they actually do. So why not just fix its shortcomings?

Then I would rather just remove that completely and force users to need to use redis/mongo/lokijs there by removing these shortcomings.

Otherwise yes I'd suggest leaving the memstore as is there is no need for a session library to have a full fledged database/memstore.

[dougwilson]

Hi @danielgindi, here are my thoughts on the topic:

In an ideal world, one which will happen soon, the core of the session module here will be extracted into a separate module. This module, express-session should glue the session core with Express, providing a session middleware abtraction (as you see the module today).

What I haven't really made up my mind on is what to do about session stories. Yes, this module currently includes one store out of the box: MemoryStore. The idea around this store what that users would start off with this module doing a single app.use() and that this store will hold on to sessions as to promote the usage of debugging tools to look into the store to understand what is going on. This is why, for example, there is a MemoryStore.prototype.all, MemoryStore.prototype.clear and MemoryStore.prototype.length, and you will not find these on typical stores, mainly because they have no real production use-case.

That being said, there is a MemoryStore indeed, and it brings with it a few gotchas, which I recently pointed out in #256 (comment) :

• You can never scale your application, not even using cluster. This is because if requests are load balanced, then you will keep getting lookup misses.
• Just one server crash and all your sessions are lost, as there is no persistence. For example, your server has a slight programming error, it crashes, your runner (forever, etc.) restarts it, but now every user is logged out of your web site.

I don't really see these shortcomings addressed in this pull request, and they are big gotchas when people create Node.js applications, especially as people deploy to services like Heroku that will automatically run multiple instances of the server. Improving the MemoryStore here is a double-edged sward, in that if MemoryStore seems to be good enough, then people will use it, and come looking for support on these issues.

Besides that, there are a lot of desires from the implementation, especially your over-use of the istanbul ignore next statement--the only acceptable use of that is for a branch that will only run under Node.js 0.8, which we do not instrument with instanbul due to install issues.

[danielgindi]

Actually the istanbul ignore next shim was there in the first place. It's there in the current express version. And that is not an issue- a shim is easily removable.

I don't get one thing. I may be the only one thinking this, but why the heck are people here trying so hard to force other people to do what they think is right?
There are many users for the memory store- because many times developers use node.js+express for fast prototyping or for making up a small webapp which runs on some local server and needs to be very simple.
Many developers need support from IT departments to have redis installed and configured and in many cases that's just too much a hassle for an app that does not need to worry about users being logged out.

I get it: the ideal is having an external memory store.
But this ideal if for a specific use case, where ut's critical not to get users logged off because of a crash, and where you load balance your server.

Devs that need to get their app to production usually understand that there are some steps they have to take to get there, and will come reading about what they have to do for it to happen.

.NET users for example- when they start with load balancing they go and read a Microsoft article, and figure out they need an SQL store or a Session Server.
In my opinion, and I guess I'm a minirity here. that's the way it is supposed to be.

[dougwilson]

Honestly I have not looked at the implementation yet, mainly because there are zero tests included in the pull request.

The initial suggestion was to publish this to npm as a store module. There is no reason we cannot remove the built-in memory store and replace it with a reference to a module.

I'm sorry there are no contribution guidelines yet, but the current pull request is missing the following items to even really be considered to merge, ignoring the debate:

1. There are no tests.
2. The code style is completely different from what it was before. I'm not going to nit on all the style issues, because I will fix when merging, but it doesn't seem like there was much attempt to even match the existing style.
3. There does not seem to be any way to stop the interval once it started, so a server can never gracefully shutdown.

If you are interested, I can go over the code in the PR after those main issues are fixed. I also don't quite understand the objection to publishing as a module to npm, either, especially because it means anyone can benefit, even people using old versions of this module and things like Express 3.

[danielgindi]

@dougwilson Thanks for your comments!
Actually there was no objection - I just had in mind that it would give an "automatic" benefit for existing users that rely on the memory store - when upgrading to express.
The whole idea is to improve things...

Regarding the tests - there are tests in the repo, being ran automatically when the PR is created. There were actually some issues that were pointed out by Travis, where the implementation did not behave exactly as expected - and I fixed those.
The coverage of the automatic tests is actually pretty great!

Regarding the code styling, I actually don't see any issues here, as I tried to keep the existing style the same (including commas on the beginning of the next line in objects, etc.). I would love it if you pointed out any styling issues here - as I myself do not like it when people are making PRs to my repos with different code style, it drives me crazy!

I will also look into the interval and figure out a way to allow graceful shutdown.

You see, even if this will end up on npm, the module have already gained benefits from being a PR here :-)

[danielgindi]

@dougwilson is there any standard function being called on the session-store when the express app is shutdown?

[dougwilson]

Hi @danielgindi, no, there is no function called on middleware when an express app is shutdown, mainly because there is no concept of an express app being shutdown; the underlying server implementation may stop listening on the socket and a user may clean up all things holding open the event loop, but Express is not a full framework that you may be imaging :) You would build a framework like that on top of Express (like SalisJS).