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

Make ConverterManager.getInstance() init thread-safe. #776

Merged
merged 2 commits into from
Apr 5, 2024

Conversation

cpovirk
Copy link
Contributor

@cpovirk cpovirk commented Mar 28, 2024

We encountered a TSAN error because 2 threads were racing to call
ConverterManager.getInstance(). My read is that the current code could
actually be unsafe, since one thread might see INSTANCE as non-null
before the other thread has initialized its fields.

Fortunately, this looks like a good case for the
initialization-on-demand holder
idiom
.

(There would be additional thread-safety concerns if anyone were to try
to mutate the ConverterManager instance concurrently with usage. But
I'm not sure there's a solution to that short of spraying synchronized
around. That seems like probably overkill, given the possible
performance impact for what I'd hope is an uncommon use case.)

We encountered a TSAN error because 2 threads were racing to call
`ConverterManager.getInstance()`. My read is that the current code could
actually be unsafe, since one thread might see `INSTANCE` as non-null
before the other thread has initialized its fields.

Fortunately, this looks like a good case for [the
initialization-on-demand holder
idiom](https://www.cs.umd.edu/~pugh/java/memoryModel/jsr-133-faq.html#:~:text=initialization%20on%20demand%20holder%20idiom).

(There would be additional thread-safety concerns if anyone were to try
to _mutate_ the `ConverterManager` instance concurrently with usage. But
I'm not sure there's a solution to that short of spraying `synchronized`
around. That seems like probably overkill, given the possible
performance impact for what I'd hope is an uncommon use case.)
@jodastephen
Copy link
Member

Looks like a good change and happy to merge, but unfortunately TestConverterManager needs fixing.

@cpovirk
Copy link
Contributor Author

cpovirk commented Apr 3, 2024

Sorry, I did not anticipate reflection in the tests :) I'll take care of it and then actually run tests, which apparently I had neglected to do.

@cpovirk
Copy link
Contributor Author

cpovirk commented Apr 3, 2024

The tests now pass.

I updated the assertion to look for the field in the new class. I now check that that class is private, but I figured that whether the field is private is mostly irrelevant in that case. Rather than throw away the field check entirely, I changed it to a check for final, since we're now able to use that.

But I'm happy to update it in some other way if you think that would be best.

@jodastephen jodastephen merged commit 34197d2 into JodaOrg:main Apr 5, 2024
4 checks passed
@jodastephen
Copy link
Member

Thanks!

@cpovirk cpovirk deleted the tsanconvertermanager branch April 5, 2024 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants