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

[dogstatsd] fix thread-safety of @timed decorator #126

Merged
merged 2 commits into from
Mar 11, 2016

Conversation

yannmh
Copy link
Member

@yannmh yannmh commented Mar 11, 2016

[dogstatsd][test] rewrite thread safety tests
Rewrite DogStatsd thread safety tests.

[dogstatsd] fix thread-safety of @timed decorator
Rebase of #112.

When using @timed as a decorator, it's not thread-safe to call itself as a context manager. The decorator creates one instance of this class, and since the context manager stores the start time on the instance, calling it again in parallel will overwrite the start time of the earlier call.
So, for the decorator version, the start time needs to be kept in the call
stack.
This includes a test case which demonstrates the problem.

Rebased to move tests to tests/unit/dogstatsd/test_statsd.py
Thanks again @mgood !

yannmh and others added 2 commits March 11, 2016 14:03
When using @timed as a decorator, it's not thread-safe to call itself as a
context manager. The decorator creates one instance of this class, and since
the context manager stores the start time on the instance, calling it again
in parallel will overwrite the start time of the earlier call.

So, for the decorator version, the start time needs to be kept in the call
stack.

[[email protected]] rebased to move tests to
`tests/unit/dogstatsd/test_statsd.py`
@mgood
Copy link

mgood commented Mar 11, 2016

👍

@yannmh yannmh self-assigned this Mar 11, 2016
yannmh added a commit that referenced this pull request Mar 11, 2016
[dogstatsd] fix thread-safety of @timed decorator
@yannmh yannmh merged commit b962275 into master Mar 11, 2016
@yannmh yannmh deleted the yann/stats-thread-safety branch March 11, 2016 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants