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

Context gets lost in Express after using session middleware #29

Open
nasooz opened this issue Nov 14, 2014 · 8 comments
Open

Context gets lost in Express after using session middleware #29

nasooz opened this issue Nov 14, 2014 · 8 comments
Labels

Comments

@nasooz
Copy link

nasooz commented Nov 14, 2014

Hi Forrest,

First of all, in my view cls is a wonderful peace of software. I just started using it and it solves lots of issues for me. Thank you for creating it and sharing it.

This is actually a questing rather than an issue. I've successfully integrated cls in an Express 4 application but after adding session middleware I started experiencing issues. To be more specific, the context gets lost. The code I've got is very similar to the following:

var express = require('express');
var session = require('express-session');   
var cls = require('continuation-local-storage');

var ns = cls.createNamespace('contextNamespace');
var app = express();

app.use(function(req, res, next) {
    ns.bindEmitter(req);
    ns.bindEmitter(res);

    ns.run(function() {
        next();
    });
});

app.use(function(req, res, next ) {
    // context is available
    var namespace = cls.getNamespace('contextNamespace');
    next();
});

// this is the critical session middleware
app.use(session({ resave: true, saveUninitialized: true, secret: 'someSecret', store: sessionStore }));

app.use(function(req, res, next ) {
    // context is lost
    var namespace = cls.getNamespace('contextNamespace');
    next();
});

As per the comments in the code above, the content gets lost for all middleware after app.use(session());

I went through all open and closed issues and I gather that in this kind of situations ns.bind() can be used but I don't quite understand how. So, I was wandering if you could give me some directions as to how to approach and solve this issue?

Thank you in advance.

I've got one other minor note. In the very first code example in your README.md, you have session.set('user', user); which doesn't seem to be enclosed in a session.run(). Is this OK or my understanding of cls is still too shallow?

Cheers,

Nasko.

@othiym23
Copy link
Owner

In the very first code example in your README.md, you have session.set('user', user); which doesn't seem to be enclosed in a session.run().

The first 2 examples are probably a little oversimplified, but what they're intended to convey is the idea that somewhere else something is calling ns.run(), and the set and get calls don't have to care, because they do their lookup from the CLS context dynamically. The first example should probably be more complete, and a patch to that effect would be welcome.

So, I was wandering if you could give me some directions as to how to approach and solve this issue?

I would have to dig into the source of express-session and see what it's doing that's interrupting the continuation chain (there are a lot of candidates, from a superficial skimming of the source – there's its custom defer function, which does some very clever, potentially CLS-breaking things under Node 0.8 and earlier; there's the monkeypatching it does of res.end and other methods). Something you could try is changing the session setup to

app.use(
  ns.bind(
    session({ 
      resave : true, 
      saveUninitialized: true,
      secret: 'someSecret',
      store: sessionStore
    })
  )
);

That may not work (see above for the cleverness going on inside express-session), in which case either you can dig in to the source yourself and try to figure out what's going on, or you can ping me, and when I have a few spare moments (probably not too soon) I can dig into it.

@wraithan or @hayes, have either of you run into this with Express 4? Might you have any pointers here?

@nasooz
Copy link
Author

nasooz commented Nov 14, 2014

Thanks for the prompt reply, Forrest. Much appreciated.

Unfortunately, the suggested app.use(ns.session())} didn't work. In my case the quick and dirty workaround is to have the cls middleware declared after the session one.

In regards to:

The first example should probably be more complete, and a patch to that effect would be welcome.

I'll see if I can come up with a suitable patch.

Cheers.

@derjust
Copy link

derjust commented Nov 28, 2014

Maybe not a real help but at least a comment:
I'm using express4 but not with the regular express-session module but with client-session.

That's working like a charm with the usual (IMHO this should be added to the README.md)

app.use(function initializeContinuationLocalStorage(req, res, next) {
    ns.bindEmitter(req);
    ns.bindEmitter(res);

    ns.run(function() {
        next();
    });
});

Therefore express itself seems to be fine and it's express-session itself causing the issue.

@askhogan
Copy link

@nasooz not sure if you typo-ed there, but wanted to highlight the differences in what you indicated didn't work and what was suggested.

@othiym23 indicated you use the ns.bind() wrapper around the already existing session function. ns.bind(session)

In your reply where you said it didnt work you wrote ns.session() , which does not use the bind method defined in the source https://github.com/othiym23/node-continuation-local-storage/blob/master/context.js

I believe ns.session() would cause an uncaught exception because session is not a method on the ns object

@forrert
Copy link

forrert commented Feb 5, 2015

@othiym23 this looks like a great module, thanks for all your work! I completely agree with you, that node needs some sort of capability for thread-local storage and that passing request/response objects through your entire application can't be the solution...

@nasooz, like you I am using this module in combination with both express and express-session and ran into the same problems with the CLS middleware not carrying through the entire request. You mentioned your workaround with adding the CLS middleware after the session middleware worked for you. Would you be willing to share a bit more of the code that worked for you? I am basically using with the same setup as the code in your first comment, except my CLS middleware is added after the session middleware. I then have another middleware that adds some property from the request object onto the active namespace (ns.set('key', value), this works) and am trying to read this property somewhere deep down the callback/promise stack (where I don't have access to the request/response objects), but the active namespace has already been removed/exited and the property is not available anymore.

Any suggestions would be greatly appreciated. Thanks!

@roberttaylortech
Copy link

This is an abbreviated chunk of a larger solution (e.g. you need to set up domains earlier on in execution) -- but I thought I would share nonetheless. Pretty sure we've nailed the issue at least for our case. Will possibly add the other code needed later. Also see my comment here: nodejs/node#66

    var domain = require("domain")
    var session = require('express-session');

    var sessionMiddleware = exports.sessionMiddleware = session({
        secret: secret,
        store: sessionMongoStore,
        rolling:true
    })

    function domainBind(fn)
    {
        if( process.domain && typeof fn === 'function'){
            fn = process.domain.bind(fn)
        }
        return fn
    }

    app.use(function(req, res, next)
    {

        var myNext = domainBind(function(){
            next();
        })

        sessionMiddleWare(req,res, myNext)

    });

@overlookmotel
Copy link
Contributor

overlookmotel commented Jun 18, 2016

Just to say that I don't think this is a bug with CLS. Just express-session is losing CLS context due to using queues internally, as many/some other libraries do. Really it needs someone to create cls-express-sessionmodule.

But I think the following should work...

Replace:

app.use( session( { /* options */ } ) );

with:

var sessionMiddleware = session( { /* options */ } );

app.use( function(req, res, next) {
    return sessionMiddleware.call( this, req, res, ns.bind(next) );
} );

@overlookmotel
Copy link
Contributor

overlookmotel commented Jun 18, 2016

A function which patches an express middleware to make it retain CLS context:

function clsifyMiddleware(fn, ns) {
    return function(req, res, next) {
        return fn.call(this, req, res, ns.bind(next));
    };
}

Then you do:

app.use( clsifyMiddleware( session( { /* options */ } ), ns ) );

NB the above doesn't re-bind res and req. But I think that isn't necessary. Maybe someone else can comment on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants