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

Cant get current accesToken in operation hook #2871

Closed
swfredrik opened this issue Oct 18, 2016 · 14 comments
Closed

Cant get current accesToken in operation hook #2871

swfredrik opened this issue Oct 18, 2016 · 14 comments

Comments

@swfredrik
Copy link

How can I get the current user in an operation hook? I've tried the guide using context vice versa but can't get it to work. Also reading that the loopback-contex is depricated.

https://docs.strongloop.com/display/public/LB/Using+current+context#Usingcurrentcontext-Configurecontextpropagation

Here is my operation hook:
Model.observe('before save', function (ctx, next) {
ctx.accessToken....
}

@fabien
Copy link
Contributor

fabien commented Oct 21, 2016

Normally, you'd use something like this:

Model.observe('before save', function (ctx, next) {
  // Note: ctx !== currentCtx
  var currentCtx = LoopBackContext.getCurrentContext();
  var accessToken = currentCtx && currentCtx.get('accessToken');
});

However, loopback-context is deprecated, and for good reason. I was bitten hard by depending on this functionality, from when it was first introduced in mid-2014. Seemed a great idea back then - just spent a couple of weeks (!) fixing all the occurrences in my(fairly large) code-base ...

@vijay22sai
Copy link

@fabien Is there anyway to access http request object inside an operation hook? How did you fix issues with loopback-context? I tried using cls-hooked but faced same issues as cls. I am looking for a workaround to replace loopback-context with a better mechanism. I am thinking of setting all context variables on req.context and consuming those values in other chain of functions. But I couldn't access http request object in operation hooks.

Can you please guide me on these lines. It would be of a great help, Thanks.

@josieusa
Copy link

josieusa commented Dec 28, 2016

@vijay22sai Hello,
regarding the package cls-hooked you mentioned, I'm curious about how did you use it.
The up-to-date approach for cls-hooked is this PR, and it's only waiting for merge (you can already try it, see the link for more info):
strongloop/loopback-context#11
The trick is to require cls-hooked before everything, but my PR adds some more stuff.
If you try it, please let me know if it works for you, thanks.

@vijay22sai
Copy link

vijay22sai commented Dec 28, 2016

@josieusa Thanks for your suggestions.

I tried to use your proposed PR as mentioned here. But still it didn't solve the concurrency issue which i have been facing.

Here is the code snippet to reproduce the issue. I have a /test endpoint which gets requestId in query object. this code works fine as expected at concurrency level 10.
package.json => "loopback-context": "github:josieusa/loopback-context#feature/cls-hooked-3",

const LoopBackContext = require('loopback-context');
app.use((req, res, next) => {
  console.log('SET query: ', req.query);
  LoopBackContext.getCurrentContext().set('query', req.query);
  next();
});

app.get('/test', (req, res, next) => {
  const promise = new Promise((resolve, reject) => {
    setImmediate(() => {
      resolve();
    });
  });

  promise.then(() => {
    console.log('get query: ', LoopBackContext.getCurrentContext().get('query'));
    res.status(200).send('ok');
  });
});

output on console for the above code looks like below (correct output):

SET query:  { randomNumber: '4965906666', rid: '37' }
SET query:  { randomNumber: '4657916975', rid: '38' }
SET query:  { randomNumber: '1995611512', rid: '39' }
get query:  { randomNumber: '4965906666', rid: '37' }
get query:  { randomNumber: '4657916975', rid: '38' }
get query:  { randomNumber: '1995611512', rid: '39' }

Now, when i replace ES6 promise with a when.js promise, context is getting messed up. At concurrency 10, only 1 or 2 requests are having original context and for other requests, context is wrong (here context is logged query). code is below.

const LoopBackContext = require('loopback-context');
const when = require('when');
app.use((req, res, next) => {
  console.log('SET query: ', req.query);
  LoopBackContext.getCurrentContext().set('query', req.query);
  next();
});

app.get('/test', (req, res, next) => {
  const promise = when.promise((resolve, reject) => {
    setImmediate(() => {
      resolve();
    });
  });

  promise.then(() => {
    console.log('get query: ', LoopBackContext.getCurrentContext().get('query'));
    res.status(200).send('ok');
  });
});

and console looks like below (wrong output):

SET query:  { randomNumber: '2942547297', rid: '35' }
SET query:  { randomNumber: '4785037287', rid: '36' }
SET query:  { randomNumber: '4075411892', rid: '37' }
SET query:  { randomNumber: '3243179957', rid: '38' }
SET query:  { randomNumber: '2582260715', rid: '39' }
get query:  { randomNumber: '2942547297', rid: '35' }
get query:  { randomNumber: '2942547297', rid: '35' }
get query:  { randomNumber: '2942547297', rid: '35' }
get query:  { randomNumber: '2942547297', rid: '35' }
get query:  { randomNumber: '2942547297', rid: '35' }

Please let me know if there is anything wrong in my usage, Thanks.

@josieusa
Copy link

@vijay22sai Thanks, I'll take a look.
Meanwhile, you may double-check if you followed the updated README:
https://github.com/josieusa/loopback-context/blob/feature/cls-hooked-3/README.md
Thanks

@josieusa
Copy link

josieusa commented Dec 28, 2016

@vijay22sai I can confirm that I get the same output :(
It's strange because, if you write your async code using a different library than when, for example async, the problem disappears (it's even tested: see https://github.com/josieusa/loopback-context/blob/feature/cls-hooked-3/test/main.test.js#L107).
I'm going to read the source code of when to discover where the context gets lost, i.e. at what line of code inside when source code, and see if it's possible to fix it or not.
I'll take care of opening an issue in loopback-context to keep track of the problem, and reproducing the bug using loopback-sandbox. Thanks a lot for pointing out the issue.

@vijay22sai
Copy link

@josieusa, Thanks for confirming the issue. Please let me know once you created the issue so that i can keep an eye on it. Also you may know that loopback-context is deprecated and #3023 is replacing it. I am waiting for #3048 patch because i am using loopback 2.x version in my application.

Were you able to get correct context in operation hooks as well? Can you test that once? Thanks a lot.

@josieusa
Copy link

@vijay22sai Yes, I know. I linked the issue I just created. No, the only way I know to do that in operation hooks is to use my PR 11 of strongloop/loopback-context, and that is what I'm using while I wait for a proper fix (maybe refactoring my app in order to stop using operation hooks, I don't know yet).

@vijay22sai
Copy link

@josieusa Thanks for creating an issue on loopback-context repo. BTW what promises library you use for async flow which works with cls-hooked? I am a heavy user of when. I want mainly this kind of functionality: when.all(bunchOfPromisesArray).resolve(resolverFunction)
Could you suggest similar promise library which also works with cls-hooked, Thanks.

@josieusa
Copy link

josieusa commented Dec 28, 2016

@vijay22sai Yes, I just tested for you thatbluebird@3.4.7 passes your concurrency test (I'm going to reproduce it in the aforementioned issue). I didn't test its Promise.all method though (similar to when.all).
(if you haven't read it yet, remember to read the updated README of loopback-context).
Luckily, bluebird is one of the most used Promise libraries, so this is good to know.

@vijay22sai
Copy link

@josieusa Thank you very much 👍 . I will test it myself and update you.

@vijay22sai
Copy link

@josieusa By using your PR and with bluebird@3.4.7, I have solved loopback-context bugs. I tested Promise.all of bluebird and it works great with context. Tested it at high concurrency (10-50 requests) and everything works great. Thanks a lot for your inputs.

I will port to use official solution whenever it is mature and stable meanwhile I will continue using your forked version. please keep me in a loop if anything interesting is happening around this area, Thank you.

@josieusa
Copy link

josieusa commented Jan 2, 2017

@vijay22sai I posted an alternative solution on strongloop/loopback-context#17

@bajtos
Copy link
Member

bajtos commented Jan 9, 2017

FWIW, #3048 has been released today. I am closing this issue, please open a new one in loopback-context if there is anything more to discuss in relation to the old getCurrentContext API.

@bajtos bajtos closed this as completed Jan 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants