Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

clear_interval doesn't delete sessions with maxAge: null #46

Closed
balupton opened this Issue · 8 comments

5 participants

@balupton

Doing some benchmarking of thousands of requests a second, there have been plenty of sessions created in mongodb that look like:

"{\"cookie\":{\"originalMaxAge\":null,\"expires\":null,\"httpOnly\":true,\"path\":\"/\"},\"passport\":{}}"  

However, connect-mongo doesn't seem to have any capabilities for removing these sessions. Setting clear_interval only wipes things which has a maxAge set not things that don't.

Any thoughts?

@kcbanner
Owner

I'll look into this, that shouldn't be null.

@porsager

I am experiencing the same thing. Have you had time to look into this?

Don't know if it is of importance, but I am using a mongoose connection with connect-mongo.

*Edit - I just tried setting the maxAge in connect config, and that resolved the issue of null sessions being created.

@cendrizzi

Mine are all being created as null, too. This used to work for me...

@cendrizzi

I found that putting this in my config to express.session added it to the cookie:
cookie : {originalMaxAge: 60000 * 60};

I'm not yet sure if it will be auto removed yet. It seems like a lot of stuff references using maxAge instead, but that didn't do anything. Has something changed (by far the hardest part of the node.js ecosystem is how much stuff changes and breaks your code).

I think this will be removed correctly because I think connect-mongo uses the expire entry which gets set correctly and has not changed. We'll see.

@aroman

Alright, I've just done my own fair bit of research into this issue. Here's where things stand, as I gather:

  • session.cookie._expires is by default null, however, because connect defaults cookie.maxAge (which in turn sets cookie._expires) to null. (See connect's session docs: http://www.senchalabs.org/connect/session.html)
  • connect-mongo only cares about session.expires (that is, session.cookie._expires) when identifying sessions to remove. This is the query: self.collection.remove({expires: {$lte: new Date()}});
  • this means that connect-mongo will simply IGNORE any sessions that have a null maxAge/expires. This is the wrong behavior. null maxAge/expires cookies should be treated, according to the IETF and connect itself, as a browser-session cookie. That is, when the user closes the browser the cookie (and session) will be removed.

So, why does connect-mongo have this problem? (Or, how do other session store implementations deal with this?)

Well, connect-redis uses redis's built-in TTL option. So redis itself handles the deletion of old sessions. If they're null, connect-redis defaults to a one-day expiration. (Source)
Connect's internal (and default) MemoryStore only seems to delete expired sessions when the session is requested (i.e in MemoryStore.prototype.get). If they're null, MemoryStore will always return them and never return them. (Source)

Basically, there is no consistency between the major SessionStore implementations about what to do here. Additionally, I don't think it's clear in the readme that "clear_interval" only determines when things that are externally marked as expired are removed, rather than being the TTL for sessions themselves. Maybe that was just my understanding of it, though.

I propose that we mirror connect-redis's implementation. Specfically, we should use mongodb 2.2's new TTL feature, which was designed for this sort of use case. I think it might also be sensible to set a one-day expiration if maxAge/expires is null, though frankly, I don't understand the rationale behind that fully.

I'd appreciate some feedback about this, particularly to hear what your original intended behavior was, @kcbanner. I'd be happy to write up a pull-request if people are in favor.

@kcbanner
Owner

@aroman clear_interval is the time between connect-mongo checking for expired sessions. It isn't related to the expiry of individual sessions, just when they get cleaned up. I added that to fix expired sessions from sticking around in the database.

I agree that we should follow connect-redis' model using the TTL feature. A pull request would be very welcome, as I haven't had much time to maintain this recently. I hope to get back in to the swing of things in the new year.

Thanks for your insight on this!

@aroman aroman referenced this issue from a commit in aroman/connect-mongo
Avi Romanoff (Fixes: #46) Use TTL sessions for removing expired sessions, set brow…
…ser-session-length expiring cookies (maxAge: null) to expire after 2 weeks of inactivity
0ccfb74
@kcbanner
Owner

I've published a new version with this change!

@kcbanner kcbanner closed this
@aroman

Woohoo! Thanks!
XeTuYX2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.