Skip to content
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

Recover a session manually when req.session is undefined #99

Closed
tciuro opened this issue Nov 20, 2014 · 20 comments
Closed

Recover a session manually when req.session is undefined #99

tciuro opened this issue Nov 20, 2014 · 20 comments
Assignees
Labels

Comments

@tciuro
Copy link

tciuro commented Nov 20, 2014

Hello,

Consider the snippet as shown in the documentation:

app.use(session( /* setup session here */ ))
app.use(function (req, res, next) {
  if (!req.session) {
    return next(new Error('oh no')) // handle error
  }
  next() // otherwise continue
})

When req.session is not available, I ping Redis and it's available. The problem is that the request has already gone through the middleware, so there's no way to recover. Would it be possible to include a function call in connect-redis to allow reloading the session manually? In other words, tell connect-redis to "try again". If we could obtain a session then, we could salvage the situation.

Once I get in this state, connect-redis never recovers. I get a !req.session on every single request... but Redis is working fine (I can access it without problems from terminal). Other than restarting the server (which sucks), is there a way to re-initialize connect-redis so that the next request can be properly initialized with a session?

We're using 1.4.7.

@dougwilson
Copy link
Contributor

What session store are you using? That store needs to have an option for retrying if it fails. This module let's the store choose how it's going to handle connection failures.

@dougwilson
Copy link
Contributor

Anyway, to answer your question, there is 100% a way to retry: send the req back through the middleware until your store module returns a session:

var sessionMiddleware = session( /* setup session here */ )

app.use(function (req, res, next) {
  var tries = 3

  function lookupSession(error) {
    if (error) {
      return next(error)
    }

    tries -= 1

    if (req.session !== undefined) {
      return next()
    }

    if (tries < 0) {
      return next(new Error('oh no'))
    }

    sessionMiddleware(req, res, lookupSession)
  }

  lookupSession()
})

@tciuro
Copy link
Author

tciuro commented Nov 20, 2014

Hi Doug,

I have added this middleware and when the issue occurs, I see the following message:

Warning: connection.session() MemoryStore is not
designed for a production environment, as it will leak
memory, and will not scale past a single process.

Why would it think I have configured a MemoryStore? This is how I have configured the express.session:

    app.use(express.session({
        name: 'session',
        secret: <some secret>,
        cookie: {
            path: '/',
            httpOnly: true,
            secure: true,
            maxAge: 24 * 60 * 60 * 1000,
            signed: false
        },
        store: new RedisStore({
            socket: <some socket>,
            prefix: <some prefix>
        })
    }));

Since the previous middleware creates the express.session, I have adjusted your code to do the following:

    app.use(function(req, res, next) {
        if (req.session) {
            return next();
        }

        var tryCount = 1,
            maxTries = 3;

        function lookupSession(error) {
            var sessionMiddleware = express.session;

            if (error) {
                return res.send(500);
            }

            console.log('Attempting to load the session: ' + tryCount + ' of ' + maxTries + '...');

            tryCount += 1;

            if (req.session) {
                return next();
            }

            if (tryCount > maxTries) {
                return res.send(500);
            }

            sessionMiddleware(req, res, lookupSession);
        }

        lookupSession();
    });

The server just gets stuck and won't continue. Any idea what's going on? I'm using Express 3. Thanks for the help!

@tciuro
Copy link
Author

tciuro commented Nov 20, 2014

I wonder if the underlying issue is that the store gets released/lost/collected and a new MemoryStore, being the default store, occupies its place... :-/

@dougwilson
Copy link
Contributor

Please follow the code above instead of the dual app.use you are trying to do. Otherwise, if you are having issues with that, post me something complete I can paste into a file unmodified and run

@dougwilson
Copy link
Contributor

Your issue is because the line

var sessionMiddleware = express.session

is completely invalid.

@tciuro
Copy link
Author

tciuro commented Nov 20, 2014

I'm not sure what you mean. Typically, expression.session needs to be inited with the session params:

app.use(express.session({
    ...
}));

You do:

var sessionMiddleware = session( /* setup session here */ )

So I can setup the session as you suggest:

var sessionMiddleware = session({
    name: 'session',
    secret: <some secret>,
    cookie: {
        path: '/',
        httpOnly: true,
        secure: true,
        maxAge: 24 * 60 * 60 * 1000,
        signed: false
    },
    store: new RedisStore({
        socket: <some socket>,
        prefix: <some prefix>
    })
})

But I need to set the session in express as well, correct? Do I set it up after your middleware?

@dougwilson
Copy link
Contributor

But I need to set the session in express as well, correct? Do I set it up after your middleware?

No. My middleware is already doing it. Replace your single app.use entirely with what I posted. It is your new session middleware app.use, basically. If you need me to write out an entire express app, let me know.

@tciuro
Copy link
Author

tciuro commented Nov 20, 2014

No, Doug, you're already doing plenty by guiding me. Many thanks for that. My issue here is that the session() function in your code is undefined. I've never instantiated a session that didn't go through express... so I'm at a loss here. Thanks again!

@dougwilson
Copy link
Contributor

My issue here is that the session() function in your code is undefined .

oohh. I just copied your code from the top:

app.use(session( /* setup session here */ ))

so I thought you would know what it was. In that case, the first line of my code can be:

var sessionMiddleware = express.session( /* setup session here */ )

@tciuro
Copy link
Author

tciuro commented Nov 20, 2014

Aha! It works now. OK, thanks. After a while I end up getting:

Error: oh no
    at lookupSession (/Users/tito/Desktop/Backup/app/app/appStart.js:140:33)
    at session (/Users/tito/Desktop/Backup/app/node_modules/express/node_modules/connect/lib/middleware/session.js:219:61)
    at lookupSession (/Users/tito/Desktop/Backup/app/app/appStart.js:143:17)
    at session (/Users/tito/Desktop/Backup/app/node_modules/express/node_modules/connect/lib/middleware/session.js:219:61)
    at lookupSession (/Users/tito/Desktop/Backup/app/app/appStart.js:143:17)
    at session (/Users/tito/Desktop/Backup/app/node_modules/express/node_modules/connect/lib/middleware/session.js:219:61)
    at lookupSession (/Users/tito/Desktop/Backup/app/app/appStart.js:143:17)
    at Object.handle (/Users/tito/Desktop/Backup/app/app/appStart.js:146:13)
    at next (/Users/tito/Desktop/Backup/app/node_modules/express/node_modules/connect/lib/proto.js:193:15)
    at Object.methodOverride [as handle] (/Users/tito/Desktop/Backup/app/node_modules/express/node_modules/connect/lib/middleware/methodOverride.js:48:5)
POST /api/auth/login 500 1ms

So three attempts and nothing, we cannot get the session.

@dougwilson
Copy link
Contributor

Ok. I can't really say :) it does just like you asked: we're calling the store module 3 times. If the store module (which is outside my knowledge and this module) still doesn't return you a session after three tries, you really need to look into the store and it's code. All this module can do it just call it more (I.e. increase the retry number).

According to the stack trace, though, we are not even calling the store for those three times because it told us to stoop calling it. Back over at the other issue, you can see the store has the ability to tell us to stop calling it until it tells us to start calling it again. It is a store module problem at that point, as we honor what the store tells us.

@dougwilson
Copy link
Contributor

By the way, what version of Express 3.x are you using?

@tciuro
Copy link
Author

tciuro commented Nov 22, 2014

Express 3.4.8

@dougwilson
Copy link
Contributor

Holy poop that is old. There is always the possibility that it's contributing to your issue. In that version of Express, express.session isn't even this module, it's this: https://github.com/senchalabs/connect/blob/2.12.0/lib/middleware/session.js

@tciuro
Copy link
Author

tciuro commented Nov 22, 2014

I need to be very careful about updating modules, as it's already in production. Which version(s) should I update? I'd like to push that through QA first. I'd like to migrate to Express 4, but unfortunately it's a slow process for us. I'd like to take care of this issue in Express 3 first. Thanks for the help!

@dougwilson
Copy link
Contributor

So, I'm not specifically recommending to update it if you don't want to, but I am saying that you are not even using this module. If you want to use this module (without upgrading your version of express, so it's a bit less risky), you can change

var express = require('express')
var app = express()
app.use(express.session({ ... })

to be

var express = require('express')
var expressSession = require('express-session')
var app = express()
app.use(expressSession({ ... }))

If there is a bug in that really old session module that is built into connect, then the only course of action would be to upgrade, though, but I'm not sure if you're actually seeing an issue in that version of express.session (unless it's interacting badly with your store module in some way).

@tciuro
Copy link
Author

tciuro commented Nov 22, 2014

I'll definitely like to upgrade if it's compatible. Question: I see that the latest released version of express-session is 1.9.1. Is it compatible with Express 3, or is it Express 4 only?

@dougwilson
Copy link
Contributor

Is it compatible with Express 3, or is it Express 4 only?

All our modules are actually compatible with just a plain http.createServer :) So the answer is yes, since we don't even require Express :D And if you end up having issues, you may want to try starting with version 1.0.0 of this module and gradually trying newer versions or something just in case.

But for compatibility, if it doesn't work with Express 3.4.8, then it's a bug.

@dougwilson
Copy link
Contributor

you may want to try starting with version 1.0.0 of this module and gradually trying newer versions

I take this back, actually, because old version of this module may potentially not be compatible, at least versions prior to 1.0.4.

@dougwilson dougwilson self-assigned this May 15, 2015
@expressjs expressjs locked and limited conversation to collaborators May 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants