From d95f15e60ed10f9dd40a8a9fc256d4ccf181c162 Mon Sep 17 00:00:00 2001 From: Erik Price Date: Fri, 18 Nov 2022 09:04:16 -0800 Subject: [PATCH 1/4] Move NumberFormat to thread-local variable 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. --- .../src/main/java/com/onthegomap/planetiler/util/Parse.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/util/Parse.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/util/Parse.java index cf2cb1db25..8243ab9034 100644 --- a/planetiler-core/src/main/java/com/onthegomap/planetiler/util/Parse.java +++ b/planetiler-core/src/main/java/com/onthegomap/planetiler/util/Parse.java @@ -19,7 +19,8 @@ public class Parse { Pattern.compile( "(?-?[\\d.]+)\\s*((?mi)|(?m|$)|(?km|kilom)|(?ft|')|(?in|\")|(?nmi|international nautical mile|nautical))", Pattern.CASE_INSENSITIVE); - private static final NumberFormat PARSER = NumberFormat.getNumberInstance(Locale.ROOT); + private static final ThreadLocal PARSER = + ThreadLocal.withInitial(() -> NumberFormat.getNumberInstance(Locale.ROOT)); private Parse() {} @@ -41,7 +42,7 @@ public static Long parseLongOrNull(String tag) { private static T retryParseNumber(Object obj, Function getter, T backup) { // more expensive parser in case simple valueOf parse fails try { - return getter.apply(PARSER.parse(obj.toString())); + return getter.apply(PARSER.get().parse(obj.toString())); } catch (ParseException e) { return backup; } From 8277036ed8faeb2011ec03eb5f243e3a73a903f3 Mon Sep 17 00:00:00 2001 From: Erik Price Date: Mon, 21 Nov 2022 09:35:10 -0800 Subject: [PATCH 2/4] Add supression for ThreadLocal use --- .../src/main/java/com/onthegomap/planetiler/util/Parse.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/util/Parse.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/util/Parse.java index 8243ab9034..3aa61949e8 100644 --- a/planetiler-core/src/main/java/com/onthegomap/planetiler/util/Parse.java +++ b/planetiler-core/src/main/java/com/onthegomap/planetiler/util/Parse.java @@ -19,6 +19,9 @@ public class Parse { Pattern.compile( "(?-?[\\d.]+)\\s*((?mi)|(?m|$)|(?km|kilom)|(?ft|')|(?in|\")|(?nmi|international nautical mile|nautical))", Pattern.CASE_INSENSITIVE); + // Ignore warnings about not removing thread local values since planetiler uses dedicated worker threads that release + // values when a task is finished and are not re-used. + @SuppressWarnings("java:S5164") private static final ThreadLocal PARSER = ThreadLocal.withInitial(() -> NumberFormat.getNumberInstance(Locale.ROOT)); From a55669d6c4a15a2bedb6c808a213bbefb34cf764 Mon Sep 17 00:00:00 2001 From: Erik Price Date: Mon, 21 Nov 2022 10:05:23 -0800 Subject: [PATCH 3/4] Use ThreadLocal for NumberFormat objects in `Format` --- .../com/onthegomap/planetiler/util/Format.java | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/util/Format.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/util/Format.java index f72139f002..a98221b34c 100644 --- a/planetiler-core/src/main/java/com/onthegomap/planetiler/util/Format.java +++ b/planetiler-core/src/main/java/com/onthegomap/planetiler/util/Format.java @@ -17,7 +17,15 @@ public class Format { public static final Locale DEFAULT_LOCALE = Locale.getDefault(Locale.Category.FORMAT); - public static final ConcurrentMap instances = new ConcurrentHashMap<>(); + + // `NumberFormat` instances are not thread safe, so we need to wrap them inside a `ThreadLocal`. + // + // Ignore warnings about not removing thread local values since planetiler uses dedicated worker threads that release + // values when a task is finished and are not re-used. + @SuppressWarnings("java:S5164") + private static final ThreadLocal> instances = ThreadLocal.withInitial( + ConcurrentHashMap::new); + private final NumberFormat pf; private final NumberFormat nf; private final NumberFormat intF; @@ -32,11 +40,7 @@ private Format(Locale locale) { } public static Format forLocale(Locale locale) { - Format format = instances.get(locale); - if (format == null) { - format = instances.computeIfAbsent(locale, Format::new); - } - return format; + return instances.get().computeIfAbsent(locale, Format::new); } public static Format defaultInstance() { From c9d0edea004f5b65d498a5a1e876b0f592e12040 Mon Sep 17 00:00:00 2001 From: Erik Price Date: Mon, 21 Nov 2022 12:25:08 -0800 Subject: [PATCH 4/4] Wrap `NumberFormat` instances rather than hashmap --- .../onthegomap/planetiler/util/Format.java | 42 ++++++++++++------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/util/Format.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/util/Format.java index a98221b34c..cd8a4a2a1a 100644 --- a/planetiler-core/src/main/java/com/onthegomap/planetiler/util/Format.java +++ b/planetiler-core/src/main/java/com/onthegomap/planetiler/util/Format.java @@ -18,29 +18,39 @@ public class Format { public static final Locale DEFAULT_LOCALE = Locale.getDefault(Locale.Category.FORMAT); + private static final ConcurrentMap instances = new ConcurrentHashMap<>(); + // `NumberFormat` instances are not thread safe, so we need to wrap them inside a `ThreadLocal`. // // Ignore warnings about not removing thread local values since planetiler uses dedicated worker threads that release // values when a task is finished and are not re-used. @SuppressWarnings("java:S5164") - private static final ThreadLocal> instances = ThreadLocal.withInitial( - ConcurrentHashMap::new); - - private final NumberFormat pf; - private final NumberFormat nf; - private final NumberFormat intF; + private final ThreadLocal pf; + @SuppressWarnings("java:S5164") + private final ThreadLocal nf; + @SuppressWarnings("java:S5164") + private final ThreadLocal intF; private Format(Locale locale) { - 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; + }); } public static Format forLocale(Locale locale) { - return instances.get().computeIfAbsent(locale, Format::new); + return instances.computeIfAbsent(locale, Format::new); } public static Format defaultInstance() { @@ -121,17 +131,17 @@ private String format(Number num, boolean pad, NavigableMap suffix /** Returns 0.0-1.0 as a "0%" - "100%" with no decimal points. */ public String percent(double value) { - return pf.format(value); + return pf.get().format(value); } /** Returns a number formatted with 1 decimal point. */ public String decimal(double value) { - return nf.format(value); + return nf.get().format(value); } /** Returns a number formatted with 0 decimal points. */ public String integer(Number value) { - return intF.format(value); + return intF.get().format(value); } /** Returns a duration formatted as fractional seconds with 1 decimal point. */