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
github-merge-queue bot referenced this pull request in camunda/camunda Sep 16, 2024
This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [joda-time:joda-time](https://www.joda.org/joda-time/)
([source](https://redirect.github.com/JodaOrg/joda-time)) | `2.12.7` ->
`2.13.0` |
[![age](https://developer.mend.io/api/mc/badges/age/maven/joda-time:joda-time/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/maven/joda-time:joda-time/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/maven/joda-time:joda-time/2.12.7/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/maven/joda-time:joda-time/2.12.7/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>JodaOrg/joda-time (joda-time:joda-time)</summary>

###
[`v2.13.0`](https://redirect.github.com/JodaOrg/joda-time/releases/tag/v2.13.0)

[Compare
Source](https://redirect.github.com/JodaOrg/joda-time/compare/v2.12.7...v2.13.0)

See the [change
notes](https://www.joda.org/joda-time/changes-report.html#a2.13.0) for
more information.

#### What's Changed

- Make `ConverterManager.getInstance()` init thread-safe. by
[@&#8203;cpovirk](https://redirect.github.com/cpovirk) in
[https://github.com/JodaOrg/joda-time/pull/776](https://redirect.github.com/JodaOrg/joda-time/pull/776)
- Add website page about secutity/CVEs by
[@&#8203;jodastephen](https://redirect.github.com/jodastephen) in
[https://github.com/JodaOrg/joda-time/pull/781](https://redirect.github.com/JodaOrg/joda-time/pull/781)
- fix: include native-image files correctly by
[@&#8203;klopfdreh](https://redirect.github.com/klopfdreh) in
[https://github.com/JodaOrg/joda-time/pull/784](https://redirect.github.com/JodaOrg/joda-time/pull/784)
- Enhance TZDB compiler to better match spec by
[@&#8203;jodastephen](https://redirect.github.com/jodastephen) in
[https://github.com/JodaOrg/joda-time/pull/786](https://redirect.github.com/JodaOrg/joda-time/pull/786)
- Update GitHub actions to latest versions by
[@&#8203;jodastephen](https://redirect.github.com/jodastephen) in
[https://github.com/JodaOrg/joda-time/pull/788](https://redirect.github.com/JodaOrg/joda-time/pull/788)
- Fix TZDB compiler %z parsing by
[@&#8203;jodastephen](https://redirect.github.com/jodastephen) in
[https://github.com/JodaOrg/joda-time/pull/787](https://redirect.github.com/JodaOrg/joda-time/pull/787)
- Update tzdb handling by
[@&#8203;jodastephen](https://redirect.github.com/jodastephen) in
[https://github.com/JodaOrg/joda-time/pull/789](https://redirect.github.com/JodaOrg/joda-time/pull/789)
- Update time zone data to 2024bgtz by
[@&#8203;github-actions](https://redirect.github.com/github-actions) in
[https://github.com/JodaOrg/joda-time/pull/790](https://redirect.github.com/JodaOrg/joda-time/pull/790)

#### New Contributors

- [@&#8203;cpovirk](https://redirect.github.com/cpovirk) made their
first contribution in
[https://github.com/JodaOrg/joda-time/pull/776](https://redirect.github.com/JodaOrg/joda-time/pull/776)
- [@&#8203;klopfdreh](https://redirect.github.com/klopfdreh) made their
first contribution in
[https://github.com/JodaOrg/joda-time/pull/784](https://redirect.github.com/JodaOrg/joda-time/pull/784)

**Full Changelog**:
JodaOrg/joda-time@v2.12.7...v2.13.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/).
View the [repository job
log](https://developer.mend.io/github/camunda/camunda).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC43NC4xIiwidXBkYXRlZEluVmVyIjoiMzguODAuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiYXV0b21lcmdlIl19-->
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.

2 participants