From f3f3f7780ed26138dfd468ca8c64dd4d16107cc7 Mon Sep 17 00:00:00 2001 From: Chris Povirk Date: Thu, 28 Mar 2024 10:07:21 -0400 Subject: [PATCH 1/2] Make `ConverterManager.getInstance()` init thread-safe. 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.) --- .../java/org/joda/time/convert/ConverterManager.java | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/joda/time/convert/ConverterManager.java b/src/main/java/org/joda/time/convert/ConverterManager.java index 2195a3726..e747986e3 100644 --- a/src/main/java/org/joda/time/convert/ConverterManager.java +++ b/src/main/java/org/joda/time/convert/ConverterManager.java @@ -81,15 +81,14 @@ public final class ConverterManager { /** - * Singleton instance, lazily loaded to avoid class loading. + * Holds the singleton instance, lazily loaded to avoid class loading. */ - private static ConverterManager INSTANCE; + private static final class LazyConverterManagerHolder { + static final ConverterManager INSTANCE = new ConverterManager(); + } public static ConverterManager getInstance() { - if (INSTANCE == null) { - INSTANCE = new ConverterManager(); - } - return INSTANCE; + return LazyConverterManagerHolder.INSTANCE; } private ConverterSet iInstantConverters; From a7b4d550ad39e9d9eff5f3f479aaf74e4a016e1b Mon Sep 17 00:00:00 2001 From: Chris Povirk Date: Wed, 3 Apr 2024 17:40:31 -0400 Subject: [PATCH 2/2] Update `TestConverterManager` for new implementation. --- .../java/org/joda/time/convert/TestConverterManager.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/joda/time/convert/TestConverterManager.java b/src/test/java/org/joda/time/convert/TestConverterManager.java index 06926f641..67e2c83f5 100644 --- a/src/test/java/org/joda/time/convert/TestConverterManager.java +++ b/src/test/java/org/joda/time/convert/TestConverterManager.java @@ -127,9 +127,12 @@ public void testSingleton() throws Exception { Constructor con = cls.getDeclaredConstructor((Class[]) null); assertEquals(1, cls.getDeclaredConstructors().length); assertEquals(true, Modifier.isProtected(con.getModifiers())); + + Class holderCls = Class.forName(cls.getName() + "$LazyConverterManagerHolder"); + assertEquals(true, Modifier.isPrivate(holderCls.getModifiers())); - Field fld = cls.getDeclaredField("INSTANCE"); - assertEquals(true, Modifier.isPrivate(fld.getModifiers())); + Field fld = holderCls.getDeclaredField("INSTANCE"); + assertEquals(true, Modifier.isFinal(fld.getModifiers())); } //-----------------------------------------------------------------------