Move NumberFormat to thread-local variable #387
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The NumberFormat class isn't thread safe, and sharing one instance across threads allowed for races which resulted in otherwise valid OSM tags not being parsed correctly.
https://github.com/onthegomap/planetiler/actions/runs/3517874164 ℹ️ Base Logs c73c835ℹ️ This Branch Logs 04ed8f7 |
From my reading of the issue, I believe this warning can be safely suppressed, but would like a second opinion. |
|
Yikes! Good catch. I'm a bit concerned about other locations that do this as well. It looks like both |
I think that's fine, can you put |
msbarry
reviewed
Nov 21, 2022
planetiler-core/src/main/java/com/onthegomap/planetiler/util/Format.java
Outdated
Show resolved
Hide resolved
|
Kudos, SonarCloud Quality Gate passed! |
msbarry
reviewed
Nov 22, 2022
Comment on lines
-26
to
+49
| pf = NumberFormat.getPercentInstance(locale); | ||
| pf.setMaximumFractionDigits(0); | ||
| nf = NumberFormat.getNumberInstance(locale); | ||
| nf.setMaximumFractionDigits(1); | ||
| intF = NumberFormat.getNumberInstance(locale); | ||
| intF.setMaximumFractionDigits(0); | ||
| pf = ThreadLocal.withInitial(() -> { | ||
| var f = NumberFormat.getPercentInstance(locale); | ||
| f.setMaximumFractionDigits(0); | ||
| return f; | ||
| }); | ||
| nf = ThreadLocal.withInitial(() -> { | ||
| var f = NumberFormat.getNumberInstance(locale); | ||
| f.setMaximumFractionDigits(1); | ||
| return f; | ||
| }); | ||
| intF = ThreadLocal.withInitial(() -> { | ||
| var f = NumberFormat.getNumberInstance(locale); | ||
| f.setMaximumFractionDigits(0); | ||
| return f; | ||
| }); |
There was a problem hiding this comment.
minor thing - you could probably just inline these in the field definitions since they don't need anything outside the initializer
There was a problem hiding this comment.
Is that possible if they need access to the locale parameter passed to the constructor?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
NumberFormatclass isn't thread safe (StackOverflow, Docs), and sharing one instance across threads allowed for races which resulted in otherwise valid OSM tags not being parsed correctly.This surfaced as some rather strange exceptions during Planetiler runs that I wasn't able to reproduce when running each input individually. For example, both of these exceptions were not reproducible in unit tests:
Given that it's a race condition, I'm not sure how to write a proper test for this, but this change reliably fixes the issue for me during local runs, and the existing tests for
Parseshould at least confirm that there aren't regressions.