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

Initial implementation #1

Merged
merged 4 commits into from
Aug 1, 2016
Merged

Initial implementation #1

merged 4 commits into from
Aug 1, 2016

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Jul 28, 2016

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 up strongRemoting.getCurrentContext and juggler.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

@raymondfeng
Copy link
Member

Do we want to name the module as loopback-context? I'm seeing some hope in Node 6.x:

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');
Copy link

@seriousben seriousben Jul 28, 2016

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.

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.

Copy link
Member Author

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.

@ritch
Copy link
Member

ritch commented Jul 28, 2016

Do we want to name the module as loopback-context

yes

2) Then you can access the context from your code:

```js
var ClsContext = require('loopback-context-cls');
Copy link
Member

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 ?

Copy link
Member Author

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.

@ritch
Copy link
Member

ritch commented Jul 28, 2016

I like this idea! This will allow us to iterate at a faster pace on this module. Eg. opt into node@6.x support of CLS (or whatever that ends up being) without having to require 6.x for LoopBack.

@bajtos bajtos force-pushed the feature/initial-implementation branch from 875d998 to 7d56227 Compare July 29, 2016 07:34
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.
@bajtos bajtos force-pushed the feature/initial-implementation branch from 7d56227 to 1f214bf Compare July 29, 2016 07:36
@bajtos
Copy link
Member Author

bajtos commented Jul 29, 2016

Changes made:

  • renamed the module to loopback-context (867a7c3)
  • renamed ClsContext to LoopBackContext in the code (867a7c3)
  • implemented lazy-loading of CLS as suggested by @seriousben (1f214bf)

@raymondfeng @ritch @seriousben LGTY now?

@raymondfeng
Copy link
Member

LGTM

josieusa added a commit to josieusa/loopback-context that referenced this pull request Jul 29, 2016
as suggested by @raymondfeng
in strongloop/loopback-context PR strongloop#1 comment #235931961
@bajtos bajtos merged commit b66ec60 into master Aug 1, 2016
@ritch
Copy link
Member

ritch commented Aug 9, 2016

👍

azatoth pushed a commit to azatoth/loopback-context that referenced this pull request Aug 16, 2016
as suggested by @raymondfeng
in strongloop/loopback-context PR strongloop#1 comment #235931961
josieusa added a commit to josieusa/loopback-context that referenced this pull request Sep 9, 2016
as suggested by @raymondfeng
in strongloop/loopback-context PR strongloop#1 comment #235931961
josieusa added a commit to josieusa/loopback-context that referenced this pull request Sep 9, 2016
as suggested by @raymondfeng
in strongloop/loopback-context PR strongloop#1 comment #235931961
josieusa added a commit to josieusa/loopback-context that referenced this pull request Sep 9, 2016
as suggested by @raymondfeng
in strongloop/loopback-context PR strongloop#1 comment #235931961
josieusa added a commit to josieusa/loopback-context that referenced this pull request Sep 12, 2016
as suggested by @raymondfeng
in strongloop/loopback-context PR strongloop#1 comment #235931961
josieusa added a commit to josieusa/loopback-context that referenced this pull request Dec 9, 2016
as suggested by @raymondfeng
in strongloop/loopback-context PR strongloop#1 comment #235931961
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

Successfully merging this pull request may close these issues.

4 participants