Skip to content

Initial implementation - #1

Merged
bajtos merged 4 commits into
masterfrom
feature/initial-implementation
Aug 1, 2016
Merged

Initial implementation#1
bajtos merged 4 commits into
masterfrom
feature/initial-implementation

Conversation

@bajtos

@bajtosbajtos commented Jul 28, 2016•
edited
Loading

Copy link
Copy Markdown
Member

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

@bajtos
bajtosforce-pushed the feature/initial-implementation branch from 47a5f1e to 875d998CompareJuly 28, 2016 13:54
@bajtosbajtos mentioned this pull request Jul 28, 2016
Merged
@raymondfeng

raymondfeng commented Jul 28, 2016

Copy link
Copy Markdown
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).

seriousben
seriousben reviewed Jul 28, 2016
View reviewed changes
Comment threadserver/current-context.js Outdated

@seriousbenseriousbenJul 28, 2016•
edited
Loading

Copy link
Copy Markdown

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.

@seriousbenseriousbenJul 28, 2016

Copy link
Copy Markdown

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.

@bajtosbajtosJul 29, 2016

Copy link
Copy Markdown
MemberAuthor

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

ritch commented Jul 28, 2016

Copy link
Copy Markdown
Member

Do we want to name the module as loopback-context

yes

ritch
ritch reviewed Jul 28, 2016
View reviewed changes
Comment threadREADME.md Outdated

@ritchritchJul 28, 2016

Copy link
Copy Markdown
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 ?

@bajtosbajtosJul 29, 2016

Copy link
Copy Markdown
MemberAuthor

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

ritch commented Jul 28, 2016

Copy link
Copy Markdown
Member

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
bajtosforce-pushed the feature/initial-implementation branch from 875d998 to 7d56227CompareJuly 29, 2016 07:34
bajtos added 2 commits July 29, 2016 09:36
@bajtos
Rename to loopback-context/LoopBackContext
867a7c3
@bajtos
Lazy-load CLS module
1f214bf
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
bajtosforce-pushed the feature/initial-implementation branch from 7d56227 to 1f214bfCompareJuly 29, 2016 07:36
@bajtos

bajtos commented Jul 29, 2016

Copy link
Copy Markdown
MemberAuthor

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?

@bajtosbajtos mentioned this pull request Jul 29, 2016
Merged
@raymondfeng

raymondfeng commented Jul 29, 2016

Copy link
Copy Markdown
Member

LGTM

josieusa added a commit to josieusa/loopback-context that referenced this pull request Jul 29, 2016
@josieusa
Use cls-hooked instead of continuation-local-storage,
518aedc
as suggested by @raymondfeng
in strongloop/loopback-context PR strongloop#1 comment #235931961
@josieusajosieusa mentioned this pull request Jul 29, 2016
Closed
@bajtos
bajtos merged commit b66ec60 into masterAug 1, 2016
@ritch

ritch commented Aug 9, 2016

Copy link
Copy Markdown
Member

👍

azatoth pushed a commit to azatoth/loopback-context that referenced this pull request Aug 16, 2016
@josieusa@azatoth
Use cls-hooked instead of continuation-local-storage,
7b79d79
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
@josieusa
Use cls-hooked instead of continuation-local-storage,
a4adcb9
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
@josieusa
Use cls-hooked instead of continuation-local-storage,
38f6110
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
@josieusa
Use cls-hooked instead of continuation-local-storage,
f39a68b
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
@josieusa
Use cls-hooked, not continuation-local-storage,
a9c8b36
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
@josieusaEmiliano Daddario
Use cls-hooked, not continuation-local-storage,
ad9bd30
as suggested by @raymondfeng
in strongloop/loopback-context PR strongloop#1 comment #235931961
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@ritchritch

@raymondfengraymondfeng

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bajtos@raymondfeng@ritch@seriousben