Bind context where it was lost

[desaroger]

I had issues (loopback#2597, loopback#2603) where I was losing context.

One solution was the usage of cls-hooked instead of cls on loopback-context as proposed by @josieusa here and that solved the issue. But when using the mongo connector, sometimes the context was losing.

I propose to patch two functions with context if available. For me this solve all my issues. I don't know if this will have downsides, but for me works always fine and all tests are passed.

PS: This is my first PR and english isn't my first language, so sorry if there are some mistakes. 😅

[slnode]

Can one of the admins verify this patch?

[slnode]

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

[slnode]

Can one of the admins verify this patch?

[codyolsen]

@desaroger Thanks for your work on this! You've pointed me in the right direction towards a fix!

[desaroger]

@codyolsen there is a plan to solve this issue in a near future and merge with master? We are using this package with the PR and I will more comfortable if it could has master installed.

My fix it's the way to solve this kind of issue or you are thinking in another form? I am fixing this kind of issue on some libraries and I am interested on learn about process.context, cls, etc.

Thanks for your response 👯

[desaroger]

I found another point where the context was losing, when updating a model through related model. I pushed another commit to this PR.

[bajtos]

I'll leave the review to @raymondfeng, @superkhau and other maintainers of this repository.

However, I personally think this patch should be rejected. We should not be monkey-patching our code like this to support CLS. Instead, the fix should be implemented in the MongoDB client library we are using under the hood. Since cls-hooked is based on AsyncWrap that's becoming the standard part of Node.js core, I expect it's fair to ask the MongoDB client to play nicely with AsyncWrap. The same way how it already supports domains.

[desaroger]

@bajtos yes I think the same, this was a desperate patch to not break our deadline.

I am a developer on Hooptap (we are using Bluemix and we are on IBM Global Entrepreneur), and our deadline is near and we needed to solve this as soon as possible, so we are using the patched version of this connector.

You know how long we would be talking until this is resolved? I am not comfortable with the idea of using my patched version instead of a official correctly-solved branch.

[superkhau]

I agree with @bajtos, this is definitely going in the wrong direction. However, @raymondfeng should have the final say here.

[jannyHou]

@desaroger Sorry to say that this is not a proper solution on loopback side.
Considering your requirement, could you fork our repo and commit your workaround?
Thanks for understanding.

[slnode]

Can one of the admins verify this patch?

[jannyHou]

I am closing it now. Please feel free to reopen it when further discussion needed. Thanks for understanding.

[bajtos]

Place for further discussions: strongloop/loopback-context#21

[pbalan]

@desaroger I tried your patch, but it doesn't work. Can you try help me here.

[desaroger]

Hi @pbalan. It was more than a year ago when I did this patch, was a bad idea to use it then and I am sure there is better solutions out there now, please check them out.

Even so, if you insist on using this patch, probably doesn't work because some change on the library since I made this patch loses the context. I would recommend you check this PR code, basically I patch any function inside .map, .foreach etc. Try to find the line where you lost your context (on the updated version of the library) and add the patch there.