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): clear call context when done #84

Merged
merged 1 commit into from
Sep 8, 2015

Conversation

ziadsawalha
Copy link
Contributor

Some threads can be reused by a bottle server and
old call data can persist in the thread-local
storage. This ensures the data is cleared after
each request.

Some threads can be reused by a bottle server and
old call data can persist in the thread-local
storage. This ensures the data is cleared after
each request.
@stavxyz
Copy link
Contributor

stavxyz commented Sep 8, 2015

The threadlocal call context will now be unavailable to any middlewares "above" the ContextMiddleware, right? Whereas previously, the context was available for the life of the thread. I don't think we've ever had code in that logical place that would depend on the threadlocal call context, but I wanted to make sure I understood correctly and document the caveat if there is one.

@ziadsawalha
Copy link
Contributor Author

The threadlocal data is available to anything in the thread. However, the .default() context will be cleared after the ContextMiddleware finishes handling its response. I make the assumption that any workers or asynchronous code running is in another thread/process/machine and would have had the context copied over by then.

Given the ContextMiddleware is what returns the X-Transaction-Id header, it should be very close to the top of the stack (if not at the top). Since any error or failure should go back with the X-Transaction-Id header in it as an ID we can use for further troubleshooting.

stavxyz added a commit that referenced this pull request Sep 8, 2015
fix(context): clear call context when done
@stavxyz stavxyz merged commit 98df2f6 into rackerlabs:master Sep 8, 2015
@ziadsawalha
Copy link
Contributor Author

Thanks @samstav

@stavxyz stavxyz mentioned this pull request Sep 9, 2015
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