add continuation-local-storage (CLS) support

[akhoury]

Ok hear me out.. you no longer need to plumb the req object through arguments, to anywhere, either for post create, or to a specific filter/action hook(s) so just you can have access to req.ip for example... all you have to do now, in ANY module, or ANY plugin is the following..

    var cls = require('./middleware/cls'); // require this once in your module.
        // ..... 

       var onReply = function () {  // or whatever
             var req = cls.get('request'); 
             // this is THE req object for THAT web request in the calls chain, 
             // you can access, ip, session info from.. whatever
             console.log(req); // from here, sky is the limit.
        }

read more about this here, most likely will become in node core eventually, or at least a very similar solution, as this extra dependency is a shim at the moment.

nodejs/node-v0.x-archive#5243
nodejs/node-v0.x-archive#6011
SHIM: https://github.com/othiym23/node-continuation-local-storage

I do not have any strong opinion on namespace name, storage key name.. or location of this extra cls.js .. please let me know what you think/feel about this.

I just hate the fact that, "sometimes" we plumb through the req object, and sometimes we don't, and when we do, it's not really part of its sibling properties in the same data hash, it's just there because we needed access to the IP address or whatever..

[julianlam]

I just hate the fact that, "sometimes" we plumb through the req object, and sometimes we don't, and when we do, it's not really part of its sibling properties in the same data hash, it's just there because we needed access to the IP address or whatever..

I agree, it was an inelegant solution to begin with, but passed our checks because there was basically no other easy option. This works, and if it's on track to be included in Node.js core, then I have no objections at all.

[barisusakli]

How does this work in places where we don't have the req object, ie socket calls.

We retrieve the uid and ip from the socket object in those places.

[akhoury]

@barisusakli i could add handler for socket connection and expose the socket the same way, .. on connection style - never tested, but will do.

io.on('connection', function(socket) {
    storage.set('socket', socket)
})

[julianlam]

Awesome, yeah please give this a shot. It would make our code more elegant, without having to explicitly pass in req/res.

[akhoury]

added websockets support - so now.. anywhere in the App or any plugin,

var cls = require('/path/to/nodebb/src/middleware/cls'),

// .... 

onWhatever: function () {
    console.log( cls.get('http') ); // prints { req: req, res: res }
    console.log( cls.get('ws') ); // prints { socket: socket, payload: payload /* if available */, event: event }
}

[julianlam]

Whoever merges will also need to add deprecation notices to all plugin hooks currently passing in req, socket, etc.

[akhoury]

i can add those first - and maybe some more documentation, where are the docs maintained ?

[psychobunny]

This is really awesome 👍

[psychobunny]

You can edit the docs right on GH, there's a link on every page that takes you to the edit page. There's a hook set up that regenerates the docs after every change

[akhoury]

that link here doesn't work yet, it's the expected link to be after you push nodebb-english live.

[akhoury]

Also, please feel free to reword that long sentence or whatever.. i am not good at that stuff.

[akhoury]

let me know what you think about this solution for the deprecation warnings - i am not too attached to it - there are too many files that needs to be touched otherwise - or we can do it on hook registrations but we'd have to make a manual list of them instead - which i could do i guess.

[akhoury]

i guess the silence means I have to manually create a list of hooks

[akhoury]

I did a search for all instances of fireHook and manually build this map of hooks, if I skipped anything please let me know or feel free to add it your self.

I think we should leave the { uid } parameter untouched, meaning limit this CLS thing to, req, res and socket, even if hooks passes the uid around, I think that's fine, because it's part of the relevant data, especially if you have interaction between 2 users, for example, follow/unfollow, or message,

I i guess, only req, res and socket should now be accessed via the CLS

Another note, for example, filter:middleware.buildHeader hook passes {req: req, locals: res.locals} as argument, I think locals should not be removed, because it's the relevant data to that hook here, even though we can access it from res.local. But req should be removed since it's not directly relevant.

[julianlam]

Okay, so what would this mean for plugins that currently subscribe to a hook that sent in these values that will be removed?

[akhoury]

This means plugins should not expect to get a reference of the req, res or socket references from the hooks' arguments anymore, but rather from the CLS module, as explained here: https://docs.nodebb.org/en/latest/plugins/create.html#getting-a-reference-to-req-res-socket-and-uid-within-any-plugin-hook

As far as the nodebb deprecation goes, we could this in 2 ways, although I wouldn't make this breaking change for 1.0, maybe some 1.x or even 2.0

1

if you merge, we could go around the src/ and remove ALL the parameters of req, res and socket (although I didn't see any socket) of ALL hooks arguments, then inside fireHook() function, we can add them back to support backward compatibility for the plugins but keep the deprecation message until you guys decide to make the breaking change - then it would be consolidated in 1 place, in about 10 lines of code to remove then.

something like:

var cls = require('../middleware/cls');

// ....
Plugins.fireHook = function(hook, params, callback) {
        // ......

               if ( isHookParamsDeprecated(hook) ) {
                      params.req = cls.get('http').req;
                      params.res = cls.get('http').res;
                      params.socket = cls.get('ws').socket;
                }

                switch (hookType) {
                 ....
                }
    };

2

if you merge, then you do nothing extra, until you decide to make the breaking change one day, then on that day, we do step 1, and remove all the req, res parameters

[julianlam]

Okay, good to merge in that case, as long as it is not a breaking change upon merge. More than willing to use deprecation notices for all 1.x.x releases, and finally deprecate in 2.x.x

This can be merged post v1.0.0 however.

[akhoury]

Ok I think this is ready for a re-review. The socket.io/index.js file might look like it was changed a lot, but it hasn't, i had to merge the latest here and I had to fix the circular dependency with CLS, so the tabbing/spaces will throw you off a bit. Sorry about that.

usage

var cls = module.parent.require('./middleware/cls'); // require cls once in your plugin.

var MyPlugin = {
        myMethod: function(postData, callback) {
            var request = cls.get('request'); // current http request object.
            var uid = request.uid; // current user id
            // ...
        },

        // let's say this one occurs on a websocket event,
        myOtherMethod: function(somethingData) {
            var request = cls.get('request'); // current returned from Sockets.reqFromSocket() object

            var uid = request.uid; // current user id, if available

            var payload = request.body; // socket payload data, if available
            var event = request.method; // socket last event, if available, not sure if using method is cool here
            var params = request.params; // socket last params, if available

            // ...
        }
    };

module.exports = MyPlugin;

[barisusakli]

Hmm, so what happens if a lot of socket events are coming in in a short period of time? Due to things being async I think there is a possibly that cls.socket() gets called twice or more before the function hook calls cls.get which would result in wrong body/method/params being reported in the hook method right?

From what I understand cls only stores one instance of the data.

[akhoury]

It won't matter, if you cls.get() within each request-call-stack*, each get() will get the corresponding data of whatever was saved somewhere up the request-call-stack.

cls only stores one instance of the data.

true, but per each request-call-stack,

*the reason I call it the request-call-stack, because the usual JS call-stack we're used to, does not include timeouts and async calls, but CLS handles all that. so if

// on some http or socket request.
doSomething() {
    var r1 = cls.get('request');

     setTimeout(function() { 
        // this timeout callback will not be on same JS call stack as doSomething, but it would be on the same request-call-stack

        var r2 = cls.get('request');

        console.log(r1 === r2); // true
    }, 100)
}

[julianlam]

@psychobunny signed off earlier
I'll sign off on this now.

Once @barisusakli approves, this is good to merge.

[akhoury]

did i break the world?

[julianlam]

You're good :)

Julian Lam

Co-Founder, NodeBB Inc.
619-341 King Street East
Toronto, Ontario
M5A 1L1
On May 9, 2016 14:14, "Aziz Khoury" notifications@github.com wrote:

did i break the world?

—
You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub
#4162 (comment)