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
Make MongoMetricsCommandListener
thread safe by not sharing Timer.Builder
#2402
Make MongoMetricsCommandListener
thread safe by not sharing Timer.Builder
#2402
Conversation
41ded52
to
220534e
Compare
@jshields-squarespace Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me. Thank you for the thorough investigation and the pull request. Would you also be able to contribute a version of the test you shared in the issue? It will be good to have such a test to ensure this doesn't regress through any future changes.
Also, if not too much trouble, could you rebase on and change the target branch to |
Sure, I can rebase and change the target branch. Regarding the test case: the reason I didn't add it to this PR is because from my understanding it's impossible to write concurrency / race condition tests that always fail in the event of a race condition. Maybe it fails on one machine, but passes on another. Or maybe it fails for some race conditions, but not all of them. So the test can be a little misleading. That said, it could still provide some value, and I'm OK adding it to this PR if that's what you'd prefer. |
I agree, it would not be a deterministic proof of lack of concurrency issue, but it could provide some possibility of catching this regressing. There's the possibility the false sense of security is a detriment, but as long as we're aware of the limitation, I think it is better to have than not. If we wanted to go hardcore on it, we could write a jcstress test, but I don't want to block getting this fix in on that. I have been meaning to add some jcstress tests for other areas of the code but have not done so yet. We can probably revisit testing this concurrency correctness with jcstress once we have another example in this codebase. |
220534e
to
72a347c
Compare
Sounds good. I'll put up another commit that adds a version of the unit test to this PR. |
@jshields-squarespace Thank you for signing the Contributor License Agreement! |
c7c181a
to
a6d312f
Compare
@shakuzen I've added a unit test for thread safety. Verified that it fails on the old code and passes on the new code. Let me know what you think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks!
As detailed in issue #2401,
MongoMetricsCommandListener
is not thread safe because it uses a shared instance ofTimer.Builder
. This means that concurrent calls tocommandSucceeded()
orcommandFailed()
can generate incorrect metrics.This PR makes
MongoMetricsCommandListener
thread safe by generating a newTimer.Builder
for each call instead of reusing a shared instance.As far as I can tell, instantiating a new
Timer.Builder
is not an expensive operation.Fixes #2401