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

Improve encapsulation test, encapsulate locals on ContextShift #807

Merged
merged 2 commits into from Jan 20, 2019

Conversation

Projects
None yet
2 participants
@oleg-py
Copy link
Collaborator

commented Jan 15, 2019

I've improved the test to cover more ways to execute Task with LCP. Found a few related to startFull not being encapsulated, if the execution starts without LCP and gets it enabled halfway through, such as task.executeWithOptions(_.enableLocalContextPropagation).unsafeRunSync().

@oleg-py oleg-py requested a review from alexandru Jan 15, 2019

@alexandru

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

@oleg-py this is interesting. However I see that you've modified the run-loop.

So what does that fix?

@oleg-py

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 15, 2019

@alexandru this is basically a continuation of #802 / #787 fix. I tried to improve test to cover more execution methods, and found that the initial fix is not enough for the cases where you don't use run...Opt variants. The case where LCP is enabled by using executeWithOptions, it would still cause the "current" thread to not reset Local value to default after task is done executing.

@alexandru

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

Interesting. OK, if this fixes the problem, then it sounds good, but I hope you tested it well.

@alexandru

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

Btw, we'll have to make Observable compatible too. There are places like the mapEval operator, that could use some magic ... i.e. internally we currently use runAsync, however if TracingScheduler is detected we could enable LCP.

@alexandru

This comment has been minimized.

Copy link
Member

commented Jan 18, 2019

Hey @oleg-py, please merge with master.

@oleg-py

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 18, 2019

Done 😄

@alexandru alexandru merged commit ce96f71 into monix:master Jan 20, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.