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

Fix context propagation broken by async@2.x #4025

Merged
merged 1 commit into from Oct 15, 2018
Merged

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Oct 11, 2018

Rework the REST middleware to use a hand-written version of "async.eachSeries". Before this change, we were loosing CLS context when the application was relying on the REST middleware to load the context middleware.

This is fixing a problem introduced by post-1.0 versions of async, which we upgraded to via fea3b78.

Related issues

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

@bajtos bajtos self-assigned this Oct 11, 2018
@bajtos bajtos requested a review from a team October 11, 2018 13:52
@hacksparrow
Copy link
Member

Why skip? Shouldn't we either make implementation changes to make it pass or remove the test itself with an explanation about its irrelevance?

@bajtos
Copy link
Member Author

bajtos commented Oct 11, 2018

Why skip? Shouldn't we either make implementation changes to make it pass or remove the test itself with an explanation about its irrelevance?

You are right, I guess I should not have been lazy. The trouble is that continuous-local-storage is notoriously difficult to get right, but at the same time I don't know if it's ok to remove all CLS/context tests from LB 2.x. I'll give this more thoughts tomorrow.

Rework the REST middleware to use a hand-written version of
"async.eachSeries". Before this change, we were loosing CLS context
when the application was relying on the REST middleware to load the
context middleware.

This is fixing a problem introduced by post-1.0 versions of async,
which we upgraded to via fea3b78.
@bajtos bajtos changed the title Skip broken tests for CLS context propagation Fix context propagation broken by async@2.x Oct 15, 2018
@bajtos
Copy link
Member Author

bajtos commented Oct 15, 2018

@hacksparrow I managed to track down the problem to a difference in behavior of async module between the version 0.9 which was used at the time we wrote the tests, and 1.x/2.x which we use now. I rewrote the relevant part of our code to use our own simplified version of async.eachSeries.

LGTY?

@hacksparrow
Copy link
Member

Looks good. 17 checks are failing, though.

@bajtos
Copy link
Member Author

bajtos commented Oct 15, 2018

17 checks are failing, though.

Most of them are outside of our control and have been failing for long time.

I checked loopback-getting-started, is has failed on nsp check (unrelated to this pull request). loopback-connector-soap is failing with "Connection fails: Error: connect ETIMEDOUT 139.59.176.153:80" (unrelated). And so on.

@bajtos bajtos merged commit b064b6d into 2.x Oct 15, 2018
@bajtos bajtos deleted the disable-context-tests branch October 15, 2018 14:44
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.

None yet

2 participants