Add context propagation middleware

[raymondfeng]

/to @ritch @bajtos @piscisaureus

[bajtos]

I don't know enough about continuation-local-storage to judge whether the implementation of the context middleware is correct. Will it work with multiple requests running in parallel?

What is the rationale for adding this feature?

I am not very happy about introducing another static/global dependency and coupling the remoted methods with the details of the REST transport. Consider the function from unit-tests:

 +    User.getToken = function(cb) {
 +      var req = process.context && process.context.get('req');
 +      cb(null, req && req.accessToken ? req.accessToken.id : null);
 +    };

How is this function supposed to be called standalone, when there is no context available? How is it supposed to work with different transports (e.g. json-rpc or websocket) that may not have req object available?

[ritch]

maybe we should use the loopback.getCurrentContext() (or similar) as public API and actually store on process or globals. This way we have the option to move around the actual location, and maintain an API.

[raymondfeng]

We somewhat need to have a global (variable/module) or access it via an existing one such as process. loopback.getCurrentContext() seems to be fine (internally we can set it to process.context.loopback too). It indicates that the context is created by loopback and only available via loopback.
I wonder how we pass the context to strong-remoting, loopback-datasource-juggler, or connectors though. There are a few use cases, such as:

1. Expose a REST API via /api/users/me (me is the current logged in user id)
2. Add a find method to a model which references the current user id
3. Use the current security context to talk to facebook or other backend systems

Maybe each module should reference the context via .getCurrentContext() and loopback can inject its implementation onto them.

[lchenay]

Is this really parallel safe ?

[ritch]

I am not very happy about introducing another static/global dependency and coupling the remoted methods with the details of the REST transport. Consider the function from unit-tests:

I think the requirement is that you can provide mock contexts for tests. In general the context should not be used inside a remote method to get at transport specific information. Instead you should use it to get the current user and other application specific contextual data.

If we provide a way to build the context for your application, you could provide one implementation for a test and others for non-tests. Something like the following:

// in my tests
app.setContext = function(context, done) {
  // includes req, res, accessToken, connection, ip, etc by default
  app.models.User.create({
    username: 'mytestuser',
    password: '**********'
  }, function(err, user) {
    // ...
    context.set('user', user);
    done();
  });
}

// my app's real implementation
app.setContext = function(context, done) {
  // includes req, res, accessToken, connection, ip, etc by default
  var userId = context.get('userId'); // available by default
  if(userId) {
    app.models.User.findById(userId, function(err, user) {
      context.set('user', user);
    });
  } else {
    process.nextTick(done);
  }
}

[ritch]

Is this really parallel safe ?

Yes.

[raymondfeng]

Any more comments?

[piscisaureus]

I'm not super excited about using continuation-local-storage. There was really no way to just pass a context object down the call chain?

[raymondfeng]

No, not all APIs allow you to pass in the context, especially across multiple modules on the call stack.

Zone can serve the same purpose but it's not available for node 0.10.

[kraman]

@piscisaureus do you know off the top, what 0.11 feature we are using which is not in 0.10? Is it possible to replace or mock out that functionality and then use Zones in loopback?

[piscisaureus]

@kraman

Zones don't use any features that are new in v 0.12.
It doesn't work on v0.10 now because the binding layer is different here and there, but there is no reason we couldn't make zones work on v10.

(This happened because initially I wanted to use async-listeners for the implementation, but that didn't work out.)

[kraman]

@raymondfeng Perhaps fixing zones to work with 0.10 is a better option in this case...

[raymondfeng]

What's the timeline? I'm very happy to use Zone for both 0.10 and 0.12.

[kraman]

@raymondfeng looking into whats not working so I can give you and chanda an estimate

[bajtos]

As I said before, I am opposed to exposing the http request & response objects to shared methods. Instead, there should be an easy and well-documented way how to write an express middleware that takes the relevant information from the request and set it on the context.

The new example code should look like this:

CartItem.sum = function(cartId, callback) {
  // ...
  var ns = loopback.getCurrentContext();
  console.log(ns.get('requestUrl'));
};

// elsewhere - add a middleware to fill the context
app.use(function(req, res, next) {
  var ns = loopback.getCurrentContext();
  ns.set('requestUrl', req.url);
  next();
});

[raymondfeng]

I have a different view here. Sure, the context should allow set/get specific context attributes. But I think there is nothing wrong by exposing req/res in the context considering a lot of express middlewares use req object as the context holder. I'm fine to introduce an option for the middleware to disable req/res.

[bajtos]

But I think there is nothing wrong by exposing req/res in the context considering a lot of express middlewares use req object as the context holder.

We are planning to support websocket based transport soon. What kind of req/resp objects is the websocket adapter going to provide in the context?

[raymondfeng]

There are going to be transport specific context for each of the protocols we support. Maybe in theory, the rest middleware should set up req/res instead.

[ritch]

There are going to be transport specific context for each of the protocols we support. Maybe in theory, the rest middleware should set up req/res instead.

The idea is that you should write methods that are unaware of the transport. This is really great for testability since you don't need to setup the transport to test the method... you just call it. It also means your code will work from node and over your desired transport. The goal is a clean separation between transport code and application code as well as being able to serialize the spec required to call the method over the transport.

The context object really helps shorten the list of arguments you have to pass to a method. It also removes the need to create a mapping to an argument (eg. the current user) that would be the same for every method.

Our goal should be that middleware (which ARE transport specific) parse out useful info (that is not transport specific) and provide that in the context.

[bajtos]

@ritch you have nailed it! 👍 💯

[chandadharap]

@raymondfeng What is your timeline on this? Clearly we shouldn't re-invent. Can you wait till after July 22nd for @kraman to dive into this? He thinks its doable, but won't know until he investigates.

[chandadharap]

Chatted with @raymondfeng Without stopping this work, we'll find time to get Zones working with 0.10, and you can switch once its released.

[lchenay]

Continuation-local-storage doesn't work with current implementation of loopback-connector-mongodb.
Namespace will be empty after first request on mongo.

[lchenay]

The use of domain instead of cls will fix this bug of broken context when a callback is called through an eventEmmiter.
Mongodb is protected since one month with this commit anton-kotenko/node-mongodb-native@66f8987

[fabien]

@lchenay I tested something similar to this https://github.com/brianc/node-domain-middleware, together with loopback/mongodb, and I can confirm this to be working.

I'm using this with this my own branch: https://github.com/fabien/loopback-connector-mongodb/tree/scoping which allows dynamic scoping during runtime, for multi-tenancy for example.

This can probably be generalized into loopback-datasource-juggler, but for now I chose to solve this at the connector-level in order to cover all the db interactions.

[ritch]

@raymondfeng ping

[pungoyal]

ping

[bajtos]

IMHO the context propagation should be added automatically by loopback() as the first middleware in the chain.

That way the minimal loopback app still keeps working:

var app = loopback();
app.use(loopback.rest());
app.listen();

What are the situations when a developer will not want to use loopback.context()? My understanding is that features like transactions will require the context , thus it is required to include the context middleware anyways.

[bajtos]

@raymondfeng comment to address ^^

[bajtos]

I am proposing the following steps to move this forward:

• remove req and res from the context, per my and @ritch comments
• ensure that the public API will allows us to replace continuation-local-storage with Zones if we decide so later
• land the patch, so that we can start getting feedback from our users

[bajtos]

See also #708

Do we merge or overwrite the settings if the loopback.rest() supplies an options argument?

Let's overwrite it. It seems to me that merge will be always confusing - is config.json overriding the values in the code? Or is it the other way around, code settings overriding config.json? I'd say the former makes more sense, as it allows per-environment overrides via config.production.json. However, it may be confusing.

[bajtos]

@raymondfeng I have reworked the integration with loopback.rest and also improved the example.

1. The example is now in a standalone file to make it clear that there is no relation between the context and the isomorphic client. Also the example shows how to correctly store http request properties in the context.
2. The integration of loopback.context and loopback.rest honours the current design where all REST-related options are configured via app.set('remoting') and/or server/config.json. That way we don't have to decide yet on how to merge multiple config objects, since there is still only a single config object.

Please review the commits I have added, possibly also the whole patch and let me know if it is good get merged. If you agree, then I'll squash your commits to clean up the history and land the PR.

[lchenay]

I'm back on the bug with cls and mongodb adapter.
With this version I still have a bug.

mongodb adapter, break domain naturally. It contain a workaround to rebind domain on callback when using function that break it. cf: https://github.com/mongodb/node-mongodb-native/blob/1.4/lib/mongodb/db.js#L1637

Cls do not use domain, and so do not use process.domain. So using cls is not fixed by this workaround.

I found a quick fix that save us. I'm sure it's not the best solution, but currently it's working. I'm creating a domain around cls, and when a bind is call on this domain, i also bind the cls namespace.
lchenay@f075c79

Other library use same fix as mongodb and rely on process.domain:

• https://github.com/petkaantonov/bluebird/blob/master/src/async.js#L35
• https://github.com/kriskowal/q/blob/v1/q.js#L1662
• https://github.com/mranney/node_redis/blob/master/index.js#L774

[bajtos]

Landed.

@lchenay could you please send a pull request with the patch you are proposing?

[crandmck]

Added #761 to document this feature. I started to add API docs, but @ritch asked me to wait, so I removed context mw from docs.json until we get further along.