Sails: connect-redis crash Sails if Redis is down and Redis password is configured

[cburatto]

Sails version: 0.12.3
Node version: 6.3.0
NPM version: 4.0.5
Operating system: Windows 10 64 bit

---

Make sure your connect-redis is properly built and installed (if using Windows, you probably need the very most recent NPM: npm install npm@next -g, besides Windows build tools https://github.com/felixrieseberg/windows-build-tools )
Setup Redis server on a separate machine
Configure Redis server to require a password
Configure Sails sessions to use connect-redis; add the proper password value
Lift Sails
All works fine
Take Sails down
Take Redis server down
Lift Sails
Sails will lift, but after a while it will crash. Crash will happen if you try to browse your site too.
Error thrown:

node_modules\connect-redis\lib\connect-redis.js:95
          throw err;
          ^
 AbortError: Redis connection lost and command aborted. It might have been processed.

Go to the connect-redis.js at that line and you will see

if (options.pass) {
      this.client.auth(options.pass, function (err) {
        if (err) {
          throw err;
        }
      });
    }

Go back to your Sails configuration, remove the password key from the session configuration (pretend your Redis server doesn't require a password).
Keep your Redis server down.
Lift Sails.
Everything works. No crash. Of course, you can't say the sessions are being handled by Redis, but at least the server is not crashing.

Previous closed issue related to this issue:

https://github.com/balderdashy/sails/issues/2276

[sailsbot]

Hi @cburatto! It looks like you missed a step or two when you created your issue. Please edit your comment (use the pencil icon at the top-right corner of the comment box) and fix the following:

• Verify "I am not asking a question about how to use Sails or about whether or not Sails has a certain feature (please refer to the documentation(http://sailsjs.org), or post on http://stackoverflow.com, our Google Group (http://bit.ly/sails-google-group) or our live chat (https://gitter.im/balderdashy/sails)."

As soon as those items are rectified, post a new comment (e.g. “Ok, fixed!”) below and we'll take a look. Thanks!

If you feel this message is in error, or you want to debate the merits of my existence (sniffle), please contact [email protected].

[cburatto]

Ok, fixed!

[mikermcneil]

@cburatto thanks for the detailed explanation! @sgress454 and I noticed this too, specifically when the Redis server disconnected while Sails was still lifted. There were also other cases related to lifting with various configuration, which I think is what you're running into here. Both of them seem to be related to the connect-redis version. Would you check what version you're using and paste it in here? Would be good to have a frame of reference.

Regardless of the version, I believe we've got it sorted out on the master branch at this point via https://github.com/balderdashy/sails/blob/master/lib/hooks/session/ensure-redis-connection.js

^^ but that won't help you until Sails v1 is released. So short term, let's take a look at your connect-redis dependency's version and try and get to the bottom of it that way.

Note: So the link's handy if it comes up, there was also a related change relatively recently to make the error message more useful: https://github.com/balderdashy/sails/commit/3f29dce22b5a36403108d8d1aab7a903aa4488a5

[cburatto]

Hi

I am using [email protected]

Please note this is on Windows 10, built by node-gyp using https://github.com/felixrieseberg/windows-build-tools

Let me know if you need more information, thanks!

[luislobo]

I've just tested it. In my case, after stopping sails, stopping redis server, and trying to lift (with a password set in sails) it just does not lift, it throws the error you mentioned.

Also, in the case of redis having a password set, but trying to start sails, it does not "crash" but if you run sails using connect-redis debug set, you actually see that AUTH is needed. Sails silently continues (but I think it should not).

The main issue with connect-redis is that it tries to run an auth method, in the contructor, and the error is thrown in the auth callback. When the auth callback is run, the sails session hook try catch block is no longer valid, thus, it just crashes the app. I managed to actually make it go through the sails error handling, but anyway, what would be the action expected? I would expect to crash anyway if redis is not available.

Just for you to try, this is my "modified" node_modules/sails/lib/hooks/session.js

/**
 * Module dependencies.
 */

var path = require('path');
var util = require('util');
var _ = require('lodash');


// generateSecret is used to generate a one-off session secret if one wasn't configured
var generateSecret = require('./generateSecret');

// (this dependency is just for creating new cookies)
var uid = require('uid-safe').sync;

// (these two dependencies are only here for sails.session.parseSessionIdFromCookie(),
//  which is only here to enable socket lifecycle callbacks)
var parseCookie = require('cookie').parse;
var stringifyCookie = require('cookie').serialize;
var unsignCookie = require('cookie-signature').unsign;
var signCookie = require('cookie-signature').sign;


module.exports = function (app) {

  var getSession = function (sessionId, errorMessage, cb) {
    app.config.session.store.get(sessionId, function (err, session) {
      if (err) return cb(err);
      if (!session) {
        return cb((function _createError() {
          var e = new Error(errorMessage);
          e.code = 'E_SESSION';
          return e;
        })());
      }
      return cb(null, session);
    });
  };
  // `session` hook definition
  var SessionHook = {


    defaults: {
      session: {
        adapter: 'memory',
        key: 'sails.sid'
      }
    },


    /**
     * Normalize and validate configuration for this hook.
     * Then fold any modifications back into `sails.config`
     */
    configure: function () {

      // Validate config
      // Ensure that secret is specified if a custom session store is used
      if (app.config.session) {
        if (!_.isObject(app.config.session)) {
          throw new Error('Invalid custom session store configuration!\n' +
            '\n' +
            'Basic usage ::\n' +
            '{ session: { adapter: "memory", secret: "someVerySecureString", /* ...if applicable: host, port, etc... */ } }' +
            '\n\nCustom usage ::\n' +
            '{ session: { store: { /* some custom connect session store instance */ }, secret: "someVerySecureString", /* ...custom settings.... */ } }'
          );
        }

      }

      // If session secret is undefined, set a secure, one-time use secret
      if (!app.config.session || !app.config.session.secret) {

        app.log.verbose('Session secret not defined-- automatically generating one for now...');

        if (app.config.environment === 'production') {
          app.log.warn('Session secret should be manually specified in production!');
          app.log.warn('Automatically generating one for now...');

          app.log.error('This generated session secret is NOT OK for production!');
          app.log.error('It will change each time the server starts and break multi-instance deployments.');
          app.log.blank();
          app.log.error('To set up a session secret, add or update it in `config/session.js`:');
          app.log.error('module.exports.session = { secret: "keyboardcat" }');
          app.log.blank();
        }

        app.config.session.secret = generateSecret();
      }

      // If secret _is_ defined, make sure it's a string
      else if (app.config.session.secret && !_.isString(app.config.session.secret)) {
        throw new Error('If provided, sails.config.session.secret should be a string.');
      }

      // Backwards-compatibility / shorthand notation
      // (allow mongo or redis session stores to be specified directly)
      if (app.config.session.adapter === 'redis') {
        app.config.session.adapter = 'connect-redis';
      }
      else if (app.config.session.adapter === 'mongo') {
        app.config.session.adapter = 'connect-mongo';
      }
    },

    /**
     * Create a connection to the configured session store
     * and keep it around
     *
     * @api private
     */
    initialize: function (cb) {
      var sessionConfig = app.config.session;
      var redisAuth = false;

      // Intepret session adapter config and "new up" a session store
      if (_.isObject(sessionConfig) && !_.isObject(sessionConfig.store)) {

        // Unless the session is explicitly disabled, require the appropriate adapter
        if (sessionConfig.adapter) {

          // 'memory' is a special case
          if (sessionConfig.adapter === 'memory') {
            var MemoryStore = require('express-session').MemoryStore;
            sessionConfig.store = new MemoryStore();
          }
          // Try and load the specified adapter from the local sails project,
          // or catch and return error:
          else {

            var COULD_NOT_REQUIRE_CONNECT_ADAPTER_ERR = function (adapter, e, onRequire) {
              var errMsg;
              if (e && typeof e === 'object' && e instanceof Error) {
                errMsg = e.stack;
              }
              else {
                errMsg = util.inspect(e);
              }

              var output = 'Could not load Connect session adapter :: ' + adapter + '\n';
              output += '\nError from adapter:\n' + errMsg + '\n\n';

              // If the error happened while attempting to require() the adapter, output some
              // (hopefully) helpful instructions on installing the adapter
              if (onRequire) {

                output += 'Do you have the Connect session adapter installed in this project?\n';
                output += 'Try running the following command in your project\'s root directory:\n';
                var installRecommendation = 'npm install ';
                installRecommendation += adapter;
                installRecommendation += '\n(Note: Make sure the version of the Connect adapter you install is compatible with Express 3/Sails v0.10)';
                installRecommendation += '\n';

                output += installRecommendation;
              }

              return output;
            };

            var SessionAdapter;
            try {
              // Attempt to initialize the adapter using the express-session module
              SessionAdapter = require(path.resolve(app.config.appPath, 'node_modules', sessionConfig.adapter));
              try {
                var CustomStore = SessionAdapter(require('express-session'));
                var auth = false;
                if (app.config.session.adapter === 'connect-redis' &&
                  app.config.session.hasOwnProperty('pass')) {
                  redisAuth = app.config.session.pass;
                  delete app.config.session.pass;
                }
                sessionConfig.store = new CustomStore(sessionConfig);
              } catch (e) {
                // If that fails, attempt to initialize the adapter using express
                // (required by older session adapters)
                try {
                  var CustomStore = SessionAdapter(require('express'));
                  sessionConfig.store = new CustomStore(sessionConfig);
                }
                catch (e2) {
                  // Failed attempting to initialize adapter; output a message w/ error info
                  // TODO: negotiate error and give better error msg depending on code
                  return cb(COULD_NOT_REQUIRE_CONNECT_ADAPTER_ERR(sessionConfig.adapter, e));
                }
              }
            } catch (e) {
              return cb(COULD_NOT_REQUIRE_CONNECT_ADAPTER_ERR(sessionConfig.adapter, e, true));
            }
          }
        }
      }

      // Expose hook as `sails.session`
      app.session = SessionHook;

      if (redisAuth) {
        return sessionConfig.store.client.auth(redisAuth, function (e) {
          if (e) {
            return cb(COULD_NOT_REQUIRE_CONNECT_ADAPTER_ERR(sessionConfig.adapter, e));
          }
          return cb();
        });
      }
      return cb();
    },


    /**
     * Generate a cookie to represent a new session.
     *
     * @return {String}
     * @api private
     */

    generateNewSidCookie: function () {
      var sid = uid(24);
      var signedSid = 's:' + signCookie(sid, app.config.session.secret);
      var cookie = stringifyCookie(app.config.session.key, signedSid, {});
      return cookie;
    },


    /**
     * Parse and unsign (i.e. decrypt) the provided cookie to get the session id.
     *
     * (adapted from code in the `express-session`)
     * (TODO: pull out into separate module as part of jshttp/pillarjs)
     *
     * @param  {String} cookie
     * @return {String}                [sessionId]
     *
     * @throws {Error} If cookie cannot be parsed or unsigned
     */
    parseSessionIdFromCookie: function (cookie) {

      // e.g. "lolcatparty"
      var sessionSecret = app.config.session.secret;

      // Parse cookie
      var parsedSidCookie = parseCookie(cookie)[app.config.session.key];

      if (typeof parsedSidCookie !== 'string') {
        throw (function createError() {
          var err = new Error('No sid cookie exists');
          err.status = 401;
          err.code = 'E_SESSION_PARSE_COOKIE';
          return err;
        })();
      }

      if (parsedSidCookie.substr(0, 2) !== 's:') {
        throw (function createError() {
          var err = new Error('Cookie unsigned');
          err.status = 401;
          err.code = 'E_SESSION_PARSE_COOKIE';
          return err;
        })();
      }

      // Unsign cookie
      var sessionId = unsignCookie(parsedSidCookie.slice(2), sessionSecret);

      if (sessionId === false) {
        throw (function createError() {
          var err = new Error('Cookie signature invalid');
          err.status = 401;
          err.code = 'E_SESSION_PARSE_COOKIE';
          return err;
        })();
      }

      return sessionId;
    },


    /**
     * @param {String} sessionId
     * @param {Function} cb
     *
     * @api private
     */
    get: function (sessionId, cb) {
      if (!_.isFunction(cb)) {
        throw new Error('Invalid usage :: `sails.hooks.session.get(sessionId, cb)`');
      }
      return getSession(sessionId, 'Session could not be loaded', cb);
    },

    /**
     * @param {String} sessionId
     * @param {} data
     * @param {Function} cb
     *
     * @api private
     */
    set: function (sessionId, data, cb) {
      if (!_.isFunction(cb)) {
        throw new Error('Invalid usage :: `sails.hooks.session.set(sessionId, data, cb)`');
      }
      return app.config.session.store.set(sessionId, data, function (err) {
        if (err) return cb(err);
        return getSession(sessionId, 'Session could not be saved', cb);
      });
    }
  };

  return SessionHook;
};

[luislobo]

for reference, quoting https://www.joyent.com/node-js/production/design/errors#fn:1

People sometimes write code like this when they want to handle the asynchronous error by invoking the callback function and passing the error as an argument. But they make the mistake of thinking that if they throw it from their own callback (the function passed to doSomeAsynchronousOperation), then it can be caught in the catch block. That's not how try/catch work with asynchronous functions. Recall that the whole point of an asynchronous function is that it's invoked some time later, after myApiFunc returns. That means the try block has been exited. The callback is invoked directly by Node, with no try block around it. So if you use this anti-pattern, you'll end up crashing the program when you throw the error.

[sailsbot]

@cburatto,@sailsbot,@mikermcneil,@luislobo: Hello, I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message and simply close the issue if possible. On the other hand, if you are still waiting on a patch, please post a comment to keep the thread alive (with any new information you can provide).

If no further activity occurs on this thread within the next 3 days, the issue will automatically be closed.

Thanks so much for your help!

[mikermcneil]

Thanks @luislobo!

but anyway, what would be the action expected? I would expect to crash anyway if redis is not available.

Well, during lift, I'd expect the lift to fail with the appropriate error msg (e.g. I'd expect the initialize callback to be invoked with an Error instance). You correctly pointed out the expected behavior and fix there, and I believe that's sorted on master (with the caveat I'm in Mongo land so I didn't go check to be sure to avoid opening too many worm cans).

As for when this happens _other times_ -- e.g. when attempting to access Redis fails at some random time during normal server operation -- that's more of a quandary. @sgress454 added new configurable functions to allow for custom handling when that happens (we ran into this during one of the big AWS downtime evens recently, for example)

[mikermcneil]

And re:

The main issue with connect-redis is that it tries to run an auth method, in the contructor, and the error is thrown in the auth callback. When the auth callback is run, the sails session hook try catch block is no longer valid, thus, it just crashes the app.

Very true! We shouldn't be doing this. Throwing in an asynchronous callback or event handler is like throwing a baseball after the batter has walked away (you just end up smashing the catcher's face)

[cburatto]

I wonder if the effect of a redis failure should really be to crash sails. Maybe only part of my app requires sessions, and I would like to keep it running in degraded mode. Redis becomes a single point of failure. Even some temporary internal network problem could drop sails.
Would there be a way to segment the apps into redis-dependent and non-dependent parts? Maybe in the routes configuration?

[luislobo]

re: reconnectiong

In our case, for MongoDB we have implemented an ugly timeout that does a count on a collection, and checking DB connection. If we get some, a hook checks a "global" flag for "connected", returns 503 for all requests with a maintenance page. Besides that, the timer tries to reconnect "gracefully" using sails.hooks.orm.reload() in some kind of exponential way up to a limit

[luislobo]

@mikermcneil is there a way to "disable" session for a route? like the way you can skipAssets... I just checked the docs (http://sailsjs.com/documentation/concepts/routes/custom-routes), but didn't see one.

[mikermcneil]

Even some temporary internal network problem could drop sails.
Would there be a way to segment the apps into redis-dependent and non-dependent parts? Maybe in the routes configuration?

@cburatto to be clear, this should only crash Sails during lift-- so in other words, it's like Sails failed to lift, except the error message isn't as good.

As for Redis failures crashing your app _post-lift_ -- that should be 100% resolved, at least in Sails v1

[mikermcneil]

is there a way to "disable" session for a route? like the way you can skipAssets... I just checked the docs, but didn't see one.

@luislobo the answer depends on sails version:

in sails v0.12.x

In Sails v0.12, you can use sails.config.session.routesDisabled:

in sails v1.x

As of Sails v1, you'll want to define a sails.config.session.isSessionDisabled function instead:

Example Sails v1 configuration

[luislobo]

@mikermcneil Oh yeah, I have just forgotten about that config option! awesome.

[mikermcneil]

It was tucked away, for sure! And it was pretty tricky to use in 0.12 sometimes since you'd have to inject the skip assets regexp route yourself (or use a list of things). The new one is a lot more powerful (and easier to copy/paste)

[luislobo]

@mikermcneil yeah, I was checking just that. One thing that we are doing, is extending the routes config using with our own keys, for different stuff, like for example, setting up rbac stuff:

DISCLAIMER: this is not official nor available in Sails, it's a custom hook used internally that handles that

'post /user': {
    controller: 'user',
    action: 'create',
    rbac: {
      'admin': {
        when: function (params, next) {
          return sails.hooks.rbac.canCreateUser(params, next);
        }
      }
    }
  },

DISCLAIMER: this is not official nor available in Sails, it's a custom hook used internally that handles that

So, in Sails 1, if you add session:false to a route config, you can easily implement it:

// In `config/session.js`
isSessionDisabled: function (req){

  if (_.has(sails.config.routes, req.route.path)) {
    return _.get(sails.config.routes[req.route.path],'session',false);
  }

   // other validations here

  // Otherwise, don't disable sessions.
  return;

}

[sgress454]

@luislobo interesting! Bear in mind that any key you put on a route like that will automatically be merged into req.options, so you don't necessarily need a separate hook to implement what you're doing. You can do this right now:

// In `config/session.js`
isSessionDisabled: function (req){

  if (req.options.useSession === false) {
    return false;
  }

   // other validations here

  // Otherwise, don't disable sessions.
  return;

}

[luislobo]

@sgress454 Nice! I didn't know that, great. Now I can simplify our RBAC hook :P

[sgress454]

Just to wrap this up, the newly-published machinepack-redis v1.3.0 correctly handles errors arising from an incorrect (or unsupplied) password upon attempting to connect to Redis, so both the sockets and session hooks in Sails should fail with an appropriate error in those cases (causing sails lift to fail as well).

[sgress454]

(leaving open until Sails 1.0 is published)

[albertpeiro]

+1 would love to see redis throw errors on lift if not connected.

[mikermcneil]

@albertpeiro 👍 This is currently working in all recent releases of sails v1, so if you install sails@beta, and then try to do this, you'll see something like:

[albertpeiro]

That's great news! A small but important improvement.

Thank you Mike

On Sat, 9 Dec 2017 at 10:00, Mike McNeil [e-mail] wrote:

@albertpeiro https://github.com/albertpeiro 👍 This is currently
working in all recent releases of sails v1, so if you install sails@beta,
and then try to do this, you'll see something like:

[image: image]
https://user-images.githubusercontent.com/618009/33794203-ef08dbfc-dc8c-11e7-8eff-a9dde9aa251e.png

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/balderdashy/sails/issues/3925#issuecomment-350433991,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB5Hjc3_P9QmxP64tG1C6myMdyfjkN3tks5s-kw2gaJpZM4LJi32
.

>

Albert Peiró

[sgress454]

🎉This was incorporated into the official 1.0 release.