Concurrent requests. #4

Closed
explodes opened this Issue Aug 2, 2011 · 16 comments

2 participants

@explodes

It appears that there are no tests that verify that two requests happening at the same time will not cause problems with "setLocale" and "getLocale."

Reading the code, it looks like there will be problems in that the module-level variable 'locale' is used.

@mashpie
Owner

yes in fact that is an issue, which should be fixed with the next pull request by cotters - feel free to provide some tests on that. Like, let them fail with current release, and hope they will pass after merge.

@mashpie
Owner

just merged the scoped hack into current master. I still need to add usage examples and tests, but you might also try to figure out by yourself. basic idea is to register the helpers in the context of a request. like:

// invoked before each action
myapp.beforeRoute = function(req, res, next) {
    // setting lang
    res.local('lang', i18n.getLocale(req));
    next();
}

myapp.someaction = function(req, res, next) {
    // scope is used in views to pass current local back to $__()
    params.scope = req;
    res.render('/someview', params);
}

to be honest, I still need to fully get codders idea ;) - but I know it is working.

@ghost

I think I'm not understanding this. In my current deploy I have that "concurrent" bug happening. I see this in my debug logs:

WARN: No locale found - check the context of the call to $__. Using de (set by request) as current locale

In addition to passing in params.scope into my template renders as indicated by mashpie's last comment, do I also need to then pass that scope into the __ function call somehow?

__.call({scope:scope}, 'Text to translate')

Seems heavy and not dry , but I'm not sure there's any other way to accomplish this?

@mashpie
Owner
@ghost

I don't understand the way that codders intended this.scope.locale to get set? I did make a further modification though that works in my case though: proksoup/i18n-node@e00acfd

I added code to the init function so it sets a function to both res.local and req on every request that has that .call({scope: request}, arguments); passed into it.

So in my templates I still use __

p __('Some text to translate')

And in my code within calls I now use req.__

app.get('/', function (req, res, next){
  res.send({
    translatedText: req.__('Some text to translate')
  });
});

I also updated my copy of the readme to reflect these changes: https://github.com/proksoup/i18n-node

If I'm heading in the right direction, I can make further changes and issue a pull request? I'm not sure if I've done more harm than good though :)

@mashpie
Owner

well, I just had a look at some old code where scope was used like:

render = function(req, res, params, next){

    [...]
    params.scope = req;
    [...]

    res.render(req.route.view, params, function(err, str){
        if(err == null) {
            res.send(str);
        } else {
            next(err);
        }
    });
}

so scope just points to current request with all parameters (locale amongst others) applied earlier in that chain. There should be an easier way to assure the current locale in any context via this.locale instead of this.scope.locale I guess. So I will need to complete portings tests and finally start to refactor this.

@ghost

I'm revisiting this issue several months later, and trying to start from scratch. I'm now using mashpie's master branch of i18n-node and am "fixing" the problem by adding a simple helper that binds the request to the scope

i18n.configure
  locales: ['en','de']
  debug: true
  directory: path.resolve __dirname + '/../conf/locales'
  app.use i18n.init
  app.use (req, res, next) ->
    __ = i18n.__.bind
      scope: req
    req.__ = __
    res.locals.__ = __
    next()

My only confusion still is why isn't something like that in init already? How are other people using this library and not getting this bug? How are others "passing that scope" ?

@mashpie
Owner

do you see any chance to get a test procedure on that issue?

@ghost

2 concurrency tests added in DocuSignDev fork.

I made mock express and request objects, to avoid the complication of adding async and depending on those projects.

The first new test adds in my "fix" in the middleware
https://github.com/DocuSignDev/i18n-node/blob/master/test/i18n.test.js#L119

The second new test verifies that the language doesn't get switched when using a "global" object. The second test fails right now.
https://github.com/DocuSignDev/i18n-node/blob/master/test/i18n.test.js#L159

@mashpie
Owner

perfect! That'll help a lot

@ghost

Those tests I just posted are dumb, they do not test concurrency. I think all of my rambling is just because I'm still not understanding how that this.scope is supposed to be set.

I added .call to my failing test and it passed, DocuSignDev@141c644

@mashpie
Owner

well this bug is serious but hard to reproduce in all setups. My bad design to register __() globally might (and will) lead to faulty language guessing on concurrent request when helpers aren't registered to req or res scope. I will definitely try to reproduce and fix it.

@mashpie
Owner

ok, now that we have some implementation examples (see https://github.com/mashpie/i18n-node#examples) I'd like to get closer to a more general solution than using individual work-arounds.

Q: any more special implementations to consider?

@ghost

Could i18n.init try to add these res.locals helpers? (since it already is added on an app.use)

(instead of needing lines 25-33 in the example)

https://gist.github.com/mashpie/5124626#file-i18n-express-cookie-app-js-L25

  // binding template helpers to request (Credits to https://github.com/enyo #12)
  app.use(function (req, res, next) {
    res.locals.__ = res.__ = function () {
      return i18n.__.apply(req, arguments);
    };
    res.locals.__n = res.__n = function () {
      return i18n.__n.apply(req, arguments);
    };
    next();
  });
@mashpie
Owner
@mashpie
Owner

done. Let's see what else will be missing

@mashpie mashpie closed this Apr 10, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment