-
Notifications
You must be signed in to change notification settings - Fork 21
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
Initial implementation #1
Conversation
47a5f1e
to
875d998
Compare
Do we want to name the module as The current implementation of getCurrentContext() is NOT reliable due to flaws in othiym23/node-continuation-local-storage#59 and other issues. If the customer is open to latest version of Node (6.x), maybe we can try an enhanced module at https://github.com/Jeff-Lewis/cls-hooked which leverages Node async-wrap (https://github.com/nodejs/node-eps/blob/async-wrap-ep/XXX-asyncwrap-api.md). |
|
||
'use strict'; | ||
|
||
var cls = require('continuation-local-storage'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if this require was only done when using CLS. As soon as this require is done, all the instrumentation/patching of async-listener is fired which is not ideal.
We've seen stack overflows due to promise instrumentation and other people have seen similar things othiym23/async-listener#57 which goes away when instrumentation is disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this could be done either in loopback or here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do the change here.
yes |
2) Then you can access the context from your code: | ||
|
||
```js | ||
var ClsContext = require('loopback-context-cls'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why ClsContext
? To differentiate between a HTTPContext
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to rename this to LoopBackContext
.
I like this idea! This will allow us to iterate at a faster pace on this module. Eg. opt into |
875d998
to
7d56227
Compare
As soon as CLS module is loaded, the instrumentation/patching of async-listener is fired. This may cause stack overflows due to promise instrumentation. By loading CLS module lazily (only when used for real), we avoid this kind of problems in applications that are not using current-context at all.
7d56227
to
1f214bf
Compare
Changes made:
@raymondfeng @ritch @seriousben LGTY now? |
LGTM |
as suggested by @raymondfeng in strongloop/loopback-context PR strongloop#1 comment #235931961
👍 |
as suggested by @raymondfeng in strongloop/loopback-context PR strongloop#1 comment #235931961
as suggested by @raymondfeng in strongloop/loopback-context PR strongloop#1 comment #235931961
as suggested by @raymondfeng in strongloop/loopback-context PR strongloop#1 comment #235931961
as suggested by @raymondfeng in strongloop/loopback-context PR strongloop#1 comment #235931961
as suggested by @raymondfeng in strongloop/loopback-context PR strongloop#1 comment #235931961
as suggested by @raymondfeng in strongloop/loopback-context PR strongloop#1 comment #235931961
Based on our discussions, I think we should remove
getCurrentContext
from LoopBack 3.0. In order to simplify upgrade path and allow community to take over maintenance of CLS-based current context, I want to follow the same process we did with loopback-boot and that is to extract the functionality into a standalone module that can be versioned independently.In this patch, I moved most of the context-related code from LoopBack to a new repository.
What was not moved:
ChainedContext
and the code setting upstrongRemoting.getCurrentContext
andjuggler.getCurrentContext
. AFAICT, these two methods were not used anywhere in strong-remoting/juggler.What's next:
@raymondfeng @ritch please review
cc @strongloop/loopback-dev FYI