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

loopback.getCurrentContext() seems to return null when MySQL is used #878

Closed
diasalvatore opened this Issue Nov 27, 2014 · 113 comments

Comments

Projects
None yet
@diasalvatore

diasalvatore commented Nov 27, 2014

As in #569 I was trying to set currentUser in context in order to retrieve it after in requests.

  app.use(loopback.token()); // this calls getCurrentContext
  app.use(loopback.context()); // the context is started here

  app.use(function (req, res, next) {
    if (!req.accessToken) return next();

    app.models.User.findById(req.accessToken.userId, function(err, user) {
      if (err)   return next(err);
      if (!user) return next(new Error('No user with this access token was found.'));
      res.locals.currentUser = user;

      var loopbackContext = loopback.getCurrentContext();
      if (loopbackContext) loopbackContext.set('currentUser', user);
      next();
    });
  });

the problem is that loopback.getCurrentContext() returns null so it will not be set.
Why does this happen?

@doublemarked

This comment has been minimized.

doublemarked commented Nov 27, 2014

I am using almost precisely the same code, also extracted from #569. It works okay for me. Where are you placing the code?

@diasalvatore

This comment has been minimized.

diasalvatore commented Nov 27, 2014

In my server.js.

I am using loopback 2.8.2, node 0.10

@doublemarked

This comment has been minimized.

doublemarked commented Nov 27, 2014

All the same on my side, sorry. If you post a more complete code snippet I can do a more thorough comparison, if you like.

@diasalvatore

This comment has been minimized.

diasalvatore commented Nov 28, 2014

Here it is: http://pastebin.com/kiHwaVeh but it's almost the default server.js, thank you.
Have I to set something in configuration somewhere?

@doublemarked

This comment has been minimized.

doublemarked commented Nov 28, 2014

There is no special configuration for it that I have done.

Can I suggest you do an npm upgrade on your project to ensure related modules are not at an incompatible version?

@diasalvatore

This comment has been minimized.

diasalvatore commented Nov 28, 2014

I did it, nothing changed.

This is the getCurrentContext() function in loopback, ns is created, but ns.active is null so it returns null.

  function createContext(scope) {
      // Make the namespace globally visible via the process.context property
      process.context = process.context || {};
      var ns = process.context[scope];
      if (!ns) {
        ns = cls.createNamespace(scope);
        process.context[scope] = ns;
        // Set up loopback.getCurrentContext()
        loopback.getCurrentContext = function() {
          return ns && ns.active ? ns : null;
        };

        chain(juggler);
        chain(remoting);
      }
      return ns;
  }

I really don't know why it doesn't work, it seems that everyone uses this code snippet

@doublemarked

This comment has been minimized.

doublemarked commented Nov 28, 2014

I have built you a very simple project which implements this routine. Everything seems to work fine. Perhaps you can use it to find what's broken in your project: http://goo.gl/CCrbgM

@diasalvatore

This comment has been minimized.

diasalvatore commented Nov 28, 2014

Thank you, you were very kind. I did the same creating a new loopback application and it works either!
How strange is that?
Now I'll put all old files in this projects and hope. Thank you again!

@diasalvatore

This comment has been minimized.

diasalvatore commented Nov 28, 2014

Could you try what happens if you link your User model to a MySQL datasource?
Because if I do this, it returns null to me

@doublemarked

This comment has been minimized.

doublemarked commented Nov 28, 2014

I'm not set up right now with a MySQL datasource, but can try perhaps in a day or two. Sounds like you may have found the difference, though.

@diasalvatore

This comment has been minimized.

diasalvatore commented Nov 28, 2014

Finally, but it costed me one day.
Here is your example with MySQL configured: https://www.dropbox.com/s/uobkp2x8bg610o6/contextTestBugged.zip?dl=0

I hope that MySQL is the cause (and if it is that someone will fix it!)

@diasalvatore diasalvatore changed the title from Problem trying to set current user in context to loopback.getCurrentContext() seems to return null when MySQL is used Nov 28, 2014

@serkanserttop

This comment has been minimized.

serkanserttop commented Nov 28, 2014

Just a suggestion, I did not test it, but could you try
app.use(loopback.context());
app.use(loopback.token());
rather than
app.use(loopback.token());
app.use(loopback.context());

@doublemarked

This comment has been minimized.

doublemarked commented Nov 29, 2014

@diasalvatore Your test project w/ a MySQL datastore indeed does fail for me also.

@bajtos

This comment has been minimized.

Member

bajtos commented Dec 1, 2014

This is most likely a duplicate of #809. There is a pending pull request #885 to fix the problem.

@bajtos

This comment has been minimized.

Member

bajtos commented Jan 14, 2015

The bug should have been fixed by #809 in loopback@2.8.8, I am closing this issue as resolved. Please leave a comment if the problem is still present in the latest loopback version.

@bajtos bajtos closed this Jan 14, 2015

@viveke10

This comment has been minimized.

viveke10 commented Mar 11, 2015

I have installed loopback@2.8.8 but still gets currentUser: null , as mentioned below

var ctx1 = loopback.getCurrentContext();
var currentUser = ctx1 && ctx1.get('currentUser');
console.log('currentUser: ', ctx1);
console.log('currentUser.username: ', currentUser);
//console.log('ctx: ', ctx); // voila!
if (ctx.instance) {
ctx.instance.updated = new Date();
} else {
ctx.data.updated = new Date();
}
next();

@bajtos

This comment has been minimized.

Member

bajtos commented Mar 11, 2015

@viveke10 please provide an app that we can use to reproduce the problem locally on our machines, see wiki instructions.

@viveke10

This comment has been minimized.

viveke10 commented Mar 12, 2015

I cannot provide you app because its client project of our company

@bajtos

This comment has been minimized.

Member

bajtos commented Mar 12, 2015

@viveke10 Have you read the instructions? We are not asking you to disclose your proprietary code, but to create a new small app that reproduces the problem.

  • Fork loopback-sandbox
  • Add the code required to reproduce your issue in the forked repository.

@bajtos bajtos reopened this Mar 12, 2015

@srguiwiz

This comment has been minimized.

srguiwiz commented May 21, 2015

Before trying the sandbox, here a report I got same (it seems) problem, with MongoDB though, even as I have the latest versions everything, with supposed fix for MongoDB.

loopback.getCurrentContext() returning null because its ns.active is null .

I found in a beforeRemoteMyMethod method a loopback.getCurrentContext() still works. It is only once it gets to the corresponding myMethod, a custom method I wrote, that it is null.

Further I have checked, it isn't the namespace it is supposed to be. I am looking in the debugger where it gets to node_modules/loopback/server/current-context.js at method loopback.createContext and am doing a debugger console test with ns.get('mystuff') and what was there before isn't there. That seems to indicate not only that it isn't active, but it isn't the same namespace that it was in my beforeRemoteMyMethod.

Got current versions couple days ago, to be sure did slc update and npm update . Then from npm list | grep loopback I have

├─┬ loopback@2.18.0
│ ├── loopback-connector-remote@1.0.3
│ ├── loopback-phase@1.2.0
├─┬ loopback-boot@2.7.1
├─┬ loopback-component-storage@1.4.0
├─┬ loopback-connector-mongodb@1.8.0
│ ├── loopback-connector@1.2.1
├─┬ loopback-datasource-juggler@2.28.0
│ ├── loopback-connector@2.1.0
├─┬ loopback-explorer@1.7.2

node --version gives v0.12.3, but was same with v0.12.2.

@srguiwiz

This comment has been minimized.

srguiwiz commented May 22, 2015

Have a reproducible case at
srguiwiz/loopback-sandbox@5349520
which is on this branch
https://github.com/srguiwiz/loopback-sandbox/commits/no-loopback-context-leo

Before that commit it was good, after that commit it wasn't.

The extra .then appears to trigger this defect, from this point of view.

I got to the reproducible case with memory db. This is equivalent to our application with a real database.

@srguiwiz

This comment has been minimized.

srguiwiz commented May 23, 2015

For that reproducible case at
srguiwiz/loopback-sandbox@5349520
a workaround by using bluebird promises instead of Node.js 0.12.3 native promises has been committed
srguiwiz/loopback-sandbox@89393a6
next on the same branch
https://github.com/srguiwiz/loopback-sandbox/commits/no-loopback-context-leo

@bajtos

This comment has been minimized.

Member

bajtos commented May 25, 2015

@srguiwiz thank you for the update. I am curious, does the same problem occur when running on io.js 2.1? It ships a much newer version of V8 which may have fixed promise-related issues like this one.

@Nivl

This comment has been minimized.

Nivl commented Jan 21, 2016

@Sandy1088 Hum, I have never used the realm and I don't know when and where getMember is called so I can't really help here. Can you create a new loopback project from scratch, try to reproduce the bug (with as few code as possible), and put it in a Github repo (so we can clone it and try it)?

@SandeshSarfare

This comment has been minimized.

SandeshSarfare commented Jan 21, 2016

Okkay I can do that soon....Thanks

@SandeshSarfare

This comment has been minimized.

SandeshSarfare commented Jan 21, 2016

@Nivl Find the repo here...Kindly add a User to produce currentUser via swagger....And let me know if you find any difficulty in comprehending the code.....Thanks Again

@SandeshSarfare

This comment has been minimized.

SandeshSarfare commented Jan 21, 2016

//Given function in repo sometimes gives currentUser and sometimes does not....Please ignore other functions
Members.getMember=function (id,cb) {
    var ctx = loopback.getCurrentContext();
    var currentUser = ctx && ctx.get('currentUser');
    console.log(currentUser);
    Members.find({"where":{MemberId:id}},function (mem_err,mem_res) {
        if (!mem_err)
         {
            cb(mem_err,mem_res);
         }

    })
}
@doublemarked

This comment has been minimized.

doublemarked commented Jan 21, 2016

@Sandy1088 code worked fine for me on node v4.2.4 and v5.3.0. It is difficult to work with the sample like this without knowing more about your environment. It would also help if the sample project were more distilled.

To anybody else - if there already exists a project which can be used to reproduce the problem, please provide a link and details to reproduce?

@Sandy1088 regarding your environment - can you please:

  1. Tell us your exact node and OS version?
  2. Shrinkwrap your node modules? Or provide a zip of your project including node_modules

Regarding project distillation - can you please make the following changes to your project, ensuring that you can reproduce the error at the end? After the changes, calling getMember should print an incrementing value on the console. If you cannot reproduce it after these changes (even when creating a user and authenticating), roll back the changes until the point that they are again reproducible, and then explain in detail what causes the shift.

// Add this new middleware to server.js: 
var reqIndex = 0;
app.use(function setContextMarker(req, res, next) {
   var loopbackContext = loopback.getCurrentContext();
   if (loopbackContext) {
      loopbackContext.set('marker', reqIndex++);
   } else {
      console.error('Missing loopbackContext in middleware from server.js');
   }
  next();
});

// Make the top of your Member.getMember function look like this:
Members.getMember=function (id,cb) {
   var ctx = loopback.getCurrentContext();
   if (ctx) { 
      console.log('Request Marker:', ctx.get('marker'));
   } else {
      console.error('Members.getMember missing Loopback Context!');
   }
   // whatever else
};

After those changes, hitting /api/Members/getMember?id=whatever should produce a message to the console without need for user or authentication. Actually there's a lot more distillation we can do than that, but this will be simple enough.

If you're having trouble reproducing this @Sandy1088 after the changes above, please drop me a PM or find me on Gitter (https://gitter.im/strongloop/loopback) and we can walk through it to try to distill a project which can be used to reliably reproduce the problem.

@Jeff-Lewis

This comment has been minimized.

Jeff-Lewis commented Jan 21, 2016

The issue of CLS losing context has little to do with Loopback, but rather the Async and Promise implementations used by Loopback and our apps. The order in which libs are required makes a BIG difference. That's why slc run might work but slc debug might not.

See how DylanSale resolved it for him with MySQL and what all of the cls-lib modules have to do fix other libraries.

Because the V8 engine does not provide a way to trace async call stack (yet hopefully), we in user-land have to hack and Shim libraries and try to keep track the async call stack which is rather difficult, especially when async and promise implementations use process.nextTick() without informing CLS.

Try your test in a way that ensures you require("continuous-local-storage") before ANYTHING else. Then make sure your all dependencies of all npm modules are using a cls wrapped version of Async, including bluebirds own asyn internal implementation.

This will make it work, but it's difficult to maintain and is a pain in the ass. That's why many of us struggle with it.

See the NG-Tracing group and @trevnorris's work on async-wrap for future developments of this.

@doublemarked

This comment has been minimized.

doublemarked commented Jan 21, 2016

@Jeff-Lewis That's interesting and makes sense. If that's the case, it does give credence to the call to deprecate use of CLS, though still leaves LB users without a clear alternative.

@fabien

This comment has been minimized.

Contributor

fabien commented Jan 21, 2016

@ALL Interesting developments - I think the main take-away is that, while we can (and probably should) deprecate the use of CLS, the functionality it offered (currentContext) should definitely be preserved going forward.

@trevnorris

This comment has been minimized.

trevnorris commented Jan 21, 2016

We're working to get AsyncWrap stable enough to be considered public API in the next couple months. Though we're still hosed in regards to the native Promise implementation. Nothing we can do ATM to observe what's going on.

@SandeshSarfare

This comment has been minimized.

SandeshSarfare commented Jan 28, 2016

I just want keep the currentUser in the context and use it for my business logic. So I use loopback.createContext() and then I add currentUser to the created context but I am not sure how will I access it in another js file. Any Ideas?

@stringbeans

This comment has been minimized.

stringbeans commented Jan 29, 2016

Could this issue affect loopback's implementation of checking whether a user in authenticated via the ACL? ie. with principalId: "$authenticated"

@Nivl

This comment has been minimized.

Nivl commented Feb 4, 2016

Just got the issue again. Updating a model in a RemoteMethod was causing the getCurrentContext in the model's access to be null.

I fixed the issue by… removing newrelic (see #1984 )

@stringbeans

This comment has been minimized.

stringbeans commented Feb 26, 2016

Should everyone be avoiding using node above v0.12.* for right now with loopback?

@tomcooksey

This comment has been minimized.

tomcooksey commented Apr 29, 2016

I have got this working on newer versions of Node, including the latest 5.9.1 - but only on certain machines - for instance my dev machine it works fine, on my colleague's it has this same problem. However, reverting to 0.12.13 version of Node it seems to work reliably across machines and servers. Obviously this is annoying as this is quite an old version of Node. Is there any update on this?

@stringbeans

This comment has been minimized.

stringbeans commented May 10, 2016

Just ran into this again with 5.10.1... the issue is very intermittent

@rwngallego

This comment has been minimized.

rwngallego commented Jun 12, 2016

If this is currently an intermittent issue why don't we change the documentation to warn this is currently not working. I have to spend some hours before I noticed it was a problem in the framework not in my own code. Also as it's intermittent it could led people to release their software with a piece of code that will be affecting in a bad manner their production software. This is unacceptable.

We should add a warning right here: https://docs.strongloop.com/display/public/LB/Using+current+context

@thanickel

This comment has been minimized.

thanickel commented Jun 29, 2016

I am seeing this issue when using promises. Every call that is made to getCurrentContext from a promise completion is causing it to be null. Not just before save, access is also returning null. Possibly others as well. I have switched to callback and is working fine. Tried with node v4.4.5 and v4.4.7 LTS versions

@CarlosCuevas

This comment has been minimized.

CarlosCuevas commented Jul 5, 2016

Never had this issue. Changed nothing but my version of node from 0.10 to 4.4.7 and it started. Using callbacks with async lib, not promises.

I added require('continuation-local-storage'); to the top of server.js and the issue seems to have gone away, though the fact that it happens intermittently makes me question the results. This issue, however, boosts my confidence in it as a fix: othiym23/node-continuation-local-storage#55

@jeff3yan

This comment has been minimized.

jeff3yan commented Jul 6, 2016

Have had this issue intermittently and can't seem to reproduce it consistently on any environment, but it's enough to discourage usage of getCurrentContext.

I'm only really using the loopback context to propagate user authentication details to a couple of dynamic role resolvers. I can consistently get around this issue by directly accessing the remotingContext in the role resolvers, although this isn't ideal as pointed out by #1495.

This means that the implementation won't be transport agnostic, but for now at least the issue seems to have disappeared completely.

@peeter-tomberg

This comment has been minimized.

peeter-tomberg commented Jul 13, 2016

Having the same issue while using promises with Transactions. Switched to callbacks and it worked. Same issue appeared in an after save hook where I used callbacks though. Turns out the issue was cause I used a promise in a asyncValidator and that messed up the after save (even though I didn't use any promises in there).

@bajtos

This comment has been minimized.

Member

bajtos commented Jan 9, 2017

Hello. The original getCurrentContext implementation was based on continuation-local-storage that's know to loose the context in many situations. We decided to deprecate this API in favour of an explicit propagation of context via the "options" argument, see #1495 and the related pull requests.

In the meantime, a new version looopback-context@3.0.0 offers a different implementation of getCurrentContext, this one is based on cls-hooked that should work better on Node 4 and newer. Feel free to give it a try. If you run into any issues with loopback-context, then please report them in loopback-context's repository on GitHub.

@bajtos bajtos closed this Jan 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment