Split MemoryStore in a separate module + avoid memory leaks

[roccomuso]

This PR addresses both #128 and #220.

The new module is fully-tested and makes use of the awesome lru-cache.

[dougwilson]

Please add Node.js 0.8 support if you want to land this here :) Also please revert the version bump from the PR; the version bump is part of the release process.

[dougwilson]

Looks like there was also a failure in Node.js 5.12 for this change as well, though I'm not sure what caused it. A second run shows 5.12 passing, so there must be an unrelated flaky test.

[dougwilson]

An additional item I noticed is that the exported MemoryStore in this library never mutated the passed-in options object. This new store is, which would be a breaking change for users who are passing in frozen objects (which seems more popular based on the issues across repos). Probably should not mutate the options object (at least to merge the change into this repo without a major version bump).

[roccomuso]

I'm adding v0.8.28 support to memorystore but I'm not sure why it's failing. On my computer ci tests are running smooth.

[dougwilson]

It looks like the module may have some variable & handle leaks. I pushed up a change to the mocha configuration so you can see the issues locally with npm test, but for posterity, here is the output of npm test on this PR:

$ npm test

> [email protected] test nodejs-express-session
> mocha --bail --no-exit --check-leaks --reporter spec test/



  session()
    √ should export constructors
    √ should do nothing if req.session exists (38ms)
    √ should error without secret
    √ should get secret from req.secret
    √ should create a new session
    √ should load session from cookie sid
    √ should pass session fetch error
    √ should treat ENOENT session fetch error as not found
    √ should create multiple sessions
    √ should handle multiple res.end calls
    √ should handle res.end(null) calls
    √ should handle reserved properties in storage
    √ should only have session data enumerable (and cookie)
    √ should not save with bogus req.sessionID
    √ should update cookie expiration when slow write (1366ms)
    when response ended
      √ should have saved session (206ms)
      √ should have saved session even with empty response (209ms)
      √ should have saved session even with multi-write (205ms)
      √ should have saved session even with non-chunked response (208ms)
      √ should have saved session with updated cookie expiration (975ms)
    when sid not in store
      √ should create a new session
      √ should have a new sid
    when sid not properly signed
      √ should generate new session
      √ should not attempt fetch from store
    when session expired in store
      √ should create a new session
      √ should have a new sid
      √ should not exist in store
    proxy option
      when enabled
        √ should trust X-Forwarded-Proto when string
        √ should trust X-Forwarded-Proto when comma-separated list
        √ should work when no header
      when disabled
        √ should not trust X-Forwarded-Proto
        √ should ignore req.secure
      when unspecified
        √ should not trust X-Forwarded-Proto
        √ should use req.secure
    cookie option
      when "secure" set to "auto"
        when "proxy" is "true"
          √ should set secure when X-Forwarded-Proto is https
        when "proxy" is "false"
          √ should not set secure when X-Forwarded-Proto is https
        when "proxy" is undefined
          √ should set secure if req.secure = true
          √ should not set secure if req.secure = false
    genid option
      √ should reject non-function values
      √ should provide default generator
      √ should allow custom function
      √ should encode unsafe chars
      √ should provide req argument
    key option
      √ should default to "connect.sid"
      √ should allow overriding
    rolling option
      √ should default to false
      √ should force cookie on unmodified session
      √ should not force cookie on uninitialized session if saveUninitialized option is set to false
      √ should force cookie and save uninitialized session if saveUninitialized option is set to true
      √ should force cookie and save modified session even if saveUninitialized option is set to false
    resave option
      √ should default to true
      √ should force save on unmodified session
      √ should prevent save on unmodified session
      √ should still save modified session
      √ should detect a "cookie" property as modified
      √ should pass session touch error
    saveUninitialized option
      √ should default to true
      √ should force save of uninitialized session
      √ should prevent save of uninitialized session
      √ should still save modified session
      √ should pass session save error
      √ should prevent uninitialized session from being touched
    secret option
      √ should reject empty arrays
      when an array
        √ should sign cookies
        √ should sign cookies with first element
        √ should read cookies using all elements
    unset option
      √ should reject unknown values
      √ should default to keep
      √ should allow destroy on req.session = null
      √ should not set cookie if initial session destroyed
      √ should pass session destroy error
    res.end patch
      √ should correctly handle res.end/res.write patched prior
      √ should correctly handle res.end/res.write patched after
    req.session
      √ should persist
      √ should only set-cookie when modified
      √ should not have enumerable methods
      √ should not be set if store is disconnected
      √ should be set when store reconnects
      .destroy()
        √ should destroy the previous session
      .regenerate()
        √ should destroy/replace the previous session
      .reload()
        √ should reload session from store
        √ should error is session missing
      .save()
        √ should save session to store
        √ should prevent end-of-request save
        √ should prevent end-of-request save on reloaded session
      .touch()
        √ should reset session expiration (115ms)
      .cookie
        .*
          √ should serialize as parameters
          √ should default to a browser-session length cookie
          √ should Set-Cookie only once for browser-session cookies
          √ should override defaults
          √ should preserve cookies set before writeHead is called
        .secure
          √ should set cookie when secure
          √ should not set-cookie when insecure
        when the pathname does not match cookie.path
          √ should not set-cookie
          √ should not set-cookie even for FQDN
        when the pathname does match cookie.path
          √ should set-cookie
          √ should set-cookie even for FQDN
        .maxAge
          √ should set cookie expires relative to maxAge
          √ should modify cookie expires when changed
          √ should modify cookie expires when changed to large value
        .expires
          when given a Date
            √ should set absolute
          when null
            √ should be a browser-session cookie
            √ should not reset cookie
            √ should not reset cookie when modified
    synchronous store
      √ should respond correctly on save
      √ should respond correctly on destroy
    cookieParser()
      √ should read from req.cookies
      √ should reject unsigned from req.cookies
      √ should reject invalid signature from req.cookies
      √ should read from req.signedCookies

  MemoryStore
    √ constructor should use default options
    √ should set options
    √ should only contain 10 items
    √ should delete the first item
    √ should delete the last item
    √ should set and get a sample entry
    √ should set TTL from cookie.maxAge (510ms)
    √ should not get empty entry
    √ should not get a deleted entry
    √ should not get an expired entry (302ms)
    √ should prune expired entries (507ms)
    √ should touch a given entry (206ms)
    √ should fetch all entries Ids
    √ should fetch all entries values
    √ should count all entries in the store
    1) should count all entries in the store


  125 passing (6s)
  1 failing

  1) MemoryStore should count all entries in the store:
     Error: global leak detected: i
      at Runner.checkGlobals (node_modules\mocha\lib\runner.js:210:21)
      at Runner.<anonymous> (node_modules\mocha\lib\runner.js:73:10)
      at node_modules\mocha\lib\runner.js:557:14
      at done (node_modules\mocha\lib\runnable.js:287:5)
      at node_modules\mocha\lib\runnable.js:360:7
      at Immediate.<anonymous> (test\session.js:2325:7)

The npm test command then just hangs forever because there is a handle leak keeping the event loop from closing.

[dougwilson]

This is the source of the i global leak.

[roccomuso]

ok thanks. Found, it was a missing local declaration.

[dougwilson]

haha, looks like we both found it :) I also just fixed the leak checks so they actually work on Travis CI -- I added them as argument to Istanbul instead of to mocha 😆

[roccomuso]

eheh yeah ;) great!

So what are you suggesting for the options object? should we clone it?

[dougwilson]

I'm adding v0.8.28 support to memorystore but I'm not sure why it's failing. On my computer ci tests are running smooth.

At least the reason it is failing on Travis CI is because npm doesn't understand what ^ is in the version.

So what are you suggesting for the options object? should we clone it?

I usually just use the pattern that this module uses, which is that all the variables are extracted out of the object, and defaults are set at extraction time. That ends up supporting two use-cases I see at once: frozen objects where you cannot modify them and object re-use, where users will pass that object around / modify it after giving it to your module and are not expecting modification. It's also much faster than cloning, especially since users also expect that they can use inherited properties and most cloning only takes into account own keys.

[roccomuso]

Could this work?

    options = options || {}
    Store.call(this, options)

    this.options = {} // < -- instead of options || {}
    this.options.checkPeriod = options.checkPeriod || oneDay
    this.options.max = options.max || Infinity
    this.options.ttl = options.ttl
    this.options.dispose = options.dispose
    this.options.stale = options.stale
    this.options.noDisposeOnSet = options.noDisposeOnSet

[roccomuso]

We should consider upgrading just the CI npm client when executing the test under Node 0.8.
The code works well with node v0.8. But that version of npm force us to downgrade the lru-cache version from v4.0.3 to v2.7.3 just because of the ^ character.

We should really consider it, like the guys of underscore did here.

[gabeio]

@dougwilson I believe you wanted the README to be 80 character wrap?

[dougwilson]

Yea

[dougwilson]

Could this work?

Yep, that would work :)

We should consider upgrading just the CI npm client when executing the test under Node 0.8. The code works well with node v0.8.

We purposely only test against the version of npm that was included in the version of Node.js at the time. If we do decide that it needs to be higher, we should list a minimum version in the engines in package.json and then that is now a major version bump of this module, as those users may find themselves unable to install all of a sudden, especially when the npm install is managed by the system.

[dougwilson]

Remove the ~ from the version here; the third-party deps are always exact versions.

[dougwilson]

Just FYI the pull request continues to fail across all Node.js versions because this change is introducing a handle leak, preventing the event loop from shutting down properly.

[dougwilson]

It looks like this replacement contains a breaking change to the MemoryStore.prototype.all method. This module's MemoryStore returns an object of session IDs to session data, but the replacement memorystore module returns an array instead.

[roccomuso]

The .all documentation says:

This optional method is used to get all sessions in the store as an array. The callback should be called as callback(error, sessions).

[dougwilson]

Sure, but that would immediately break anyone who is today using the MemoryStore reference, a breaking change regardless.

[dougwilson]

Looks like I just documented it wrong when I added the docs; the implementation existed for a very long time before the docs were added. The doc issue looks to have been an oversight, we can fix the docs :)

[roccomuso]

Alright, fixing it right now, btw are all the contributors' store modules being using Array or Object? :)

PS. I'm also trying to investigate who's causing the handle leak.

[dougwilson]

Alright, fixing it right now, btw are all the contributors' store modules being using Array or Object? :)

I'm not sure, because there are a lot to look through. I would assume they are using Array since that is the only form that was ever documented. You can also use Array in your module too, it's only since you're trying to actually replace the existing MemoryStore that makes it matter. If your module is just listed with the others, then it really doesn't matter.

Another bug I just found is that the session data in your store is not getting serialized and/or cloned between requests. This means that the exact same object reference is given to multiple requests, and mutating the object in out requests causes another request that is happening in parallel to also mutate. This is not an issue in the current MemoryStore, though. Please take a look at fixing that :)

[dougwilson]

Another issue I just found is that the store is not honoring the resave: false option; it looks like when that is set, the call to touch still smashes over the entire session object, even when it was unmodified. This creates a race conditions where a client makes two parallel requests to your server and changes made to the session in one request may get overwritten when the other request ends, even if it made no change.

I'm running your module inside a product app right now to help smoketest the rewritten implementation. I'll report if I find anything else.

[roccomuso]

As regards the resave: false option. Neither connect-redis is supporting it, right?

// connect-redis
  RedisStore.prototype.touch = function (sid, sess, fn) {
    var store = this;
    var psid = store.prefix + sid;
    if (!fn) fn = noop;
    if (store.disableTTL) return fn();

    var ttl = getTTL(store, sess);

    debug('EXPIRE "%s" ttl:%s', sid, ttl);
    store.client.expire(psid, ttl, function (er) {
      if (er) return fn(er);
      debug('EXPIRE complete');
      fn.apply(this, arguments);
    });
  };

and this is my touch() method right now:

// memorystore
  MemoryStore.prototype.touch = function (sid, sess, fn) {
    var store = this.store

    var ttl = getTTL(store, sess, sid)

    debug('EXPIRE "%s" ttl:%s', sid, ttl)
    store.set(sid, store.get(sid) || sess, ttl)
    fn && defer(fn, null)
  }

[dougwilson]

Neither connect-redis is supporting it, right?

I'm only comparing this to the MemoryStore implementation you're replacing. If your module is just listed with the others, then it really doesn't matter, but since this pull request is replacing the existing MemoryStore, it needs to be compatible at support at least what the implementation it is replacing supports.

[roccomuso]

Could the handle leak be related to coveralls pheraps?
I can't reproduce it locally, my builds are exiting correctly.

Edit: I can't reproduce it on a separate branch with travis too :') The build now worked. It could easily be a flaky Travis issue.

Btw can you share/add some test for the resave: false option?
Otherwise I'm not sure if I solved it or not.

[dougwilson]

I'm traveling right now so cannot take a close look, but I guarantee you the leak is real. Tou can see I wrote about it way above, showing it on my own machine. Likely the reason you are not seeing it locally is because yiur branch does not contain the check I added to master to show the issue :) I noted above I pushed a change to master so the test suite would now catch variable and handle leaks. You need to fix the handke leak in your code.

I will add a twst case when I get back late next week for ya.

[roccomuso]

ops, I didn't notice your commit with the --no-exit mocha param ;)
Now the leak is fixed. I've also changed the default behavior to not have a periodic Interval that removes the expired entries. If you want it, you should provide a checkPeriod option to the store.

[gabeio]

Please wrap the README.md to 80 characters

[gabeio]

Please wrap at 80 characters.

[roccomuso]

done

[dougwilson]

I still think that we should just list your module with the others and add a note up at the top with the production warning instead of actually replacing our implementation. I'm not sure if even adding this module as our dependency is going to work out in the long run, as just in the course of this pull request the module has made several breaking changes without any major version bumps, a violation of semver. I assume that that means this is not a semver-compliant module, which isn't something we really want to depend on.

Here is what my proposal is, summarizing a few previous comments above:

1. Add your great module to the list of session stores.
2. Add an additional note to the default session memory warning in the readme at the end that if you actually want to use a memory store in production, to use the memorystore module.

Then as a separate step, we can deprecate users not providing a store option.

Even the memorystore module's readme includes the following:

The automatic check is disabled by default! Not setting this is kind of silly, since that's the whole purpose of this lib.

So re-enforcing that the users should be creating a memory store instance themselves and passing it to the store option, at which point there isn't a reason that the memorystore module needs to be our dependency. We found with Express 3 that having dependencies is hard on even the dependencies. For example, if your memorystore becomes our dependency, who knows when you'll ever be able to making a breaking change; it will harm the ability for you to improve and work on your own module, which sucks. That's why body-parser, morgan, and more and not dependencies of Express.

I hope this makes sense. What do you think @gabeio ?

[gabeio]

Sounds good to me. Having as few dependencies as possible is very nice. It especially sounds like a good idea to deprecate the in memory database on this module (and eventually remove it).

[roccomuso]

Ok but just consider that this module is written in a way that doesn't break things. It's compliant with the old MemoryStore APIs and behaviour right now. Tests should be written to prove this at least and all the checks passed.
The WARN is ok, but if you have the chance to get rid of it with a valid substitute would be less annoying for the user I guess.

[dougwilson]

Ok but just consider that this module is written in a way that doesn't break things. It's compliant with the old MemoryStore APIs and behaviour right now. Tests should be written to prove this at least and all the checks passed.

Right, but that has nothing to do with the proposal above.

The WARN is ok, but if you have the chance to get rid of it with a valid substitute would be less annoying for the user I guess.

But we don't want to have any store be part of this module at all. Just like Express dropped including all middleware in 4.0.

[dougwilson]

Previous PRs improving the MemoryStore never landed because we don't want to improve it and just want it gone. This is going in the opposite direction we really want, and @gabeio agrees. I think that we should go down the proposed route above to direct users to your module harder and get the MemoryStore removed from our code base. We never removed the MemoryStore module from our code base partly because until now, there was no store module to direct those users to, but now hat you made memorystore, that's no longer the case.

[roccomuso]

ok that's clear, it make sense. Let's summerize the next steps then. :)

If memorystore is kept separated, I'd suggest to bump the lru-cache dependency to the latest version and remove support for node v0.8.

[dougwilson]

Right, if your module is just listed with the others, then it really doesn't matter. Supporting Node.js 0.8 was only necessary if it was a dependency of this module. Being free to choose what Node.js versions you want to support is one of the nice things about not being a dependency. This was one of the same reasons middleware was removed in Express 4.

[roccomuso]

Ok, so I could revert the commits or better open another PR with the warning mex and the updated documentation. What do you think?

[dougwilson]

Either one is fine. You're welcome to continue to use this PR

[dougwilson]

I haven't heard back in a while, so I went ahead and (for now) added the memorystore module to the list in the README.

[roccomuso]

Sorry for my late response. Thanks for all the hard work.
I think we can proceed and remove MemoryStore from the codebase then as you suggested.