From 07b27f1ee7ca348d7351ab436781acbf485d18d0 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Tue, 1 Dec 2020 23:11:27 +0100 Subject: [PATCH 1/7] Fix non-threadsafe creation of adapter for type with cyclic dependency --- gson/src/main/java/com/google/gson/Gson.java | 44 ++++++++++++++------ 1 file changed, 31 insertions(+), 13 deletions(-) diff --git a/gson/src/main/java/com/google/gson/Gson.java b/gson/src/main/java/com/google/gson/Gson.java index 27f3ee9246..446149c7a5 100644 --- a/gson/src/main/java/com/google/gson/Gson.java +++ b/gson/src/main/java/com/google/gson/Gson.java @@ -32,6 +32,7 @@ import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicLongArray; @@ -124,7 +125,7 @@ public final class Gson { private final ThreadLocal, FutureTypeAdapter>> calls = new ThreadLocal, FutureTypeAdapter>>(); - private final Map, TypeAdapter> typeTokenCache = new ConcurrentHashMap, TypeAdapter>(); + private final ConcurrentMap, TypeAdapter> typeTokenCache = new ConcurrentHashMap, TypeAdapter>(); private final ConstructorConstructor constructorConstructor; private final JsonAdapterAnnotationTypeAdapterFactory jsonAdapterFactory; @@ -447,7 +448,8 @@ public TypeAdapter getAdapter(TypeToken type) { // the key and value type parameters always agree FutureTypeAdapter ongoingCall = (FutureTypeAdapter) threadCalls.get(type); if (ongoingCall != null) { - return ongoingCall; + TypeAdapter resolvedAdapter = ongoingCall.delegate; + return resolvedAdapter != null ? resolvedAdapter : ongoingCall; } try { @@ -458,14 +460,27 @@ public TypeAdapter getAdapter(TypeToken type) { TypeAdapter candidate = factory.create(this, type); if (candidate != null) { call.setDelegate(candidate); - typeTokenCache.put(type, candidate); + + if (requiresThreadLocalCleanup) { + // Publish resolved adapters to all threads + // Can only do this for the initial request because cyclic dependency TypeA -> TypeB -> TypeA + // would otherwise publish adapter for TypeB which uses not yet resolved adapter for TypeA + // See https://github.com/google/gson/issues/625 + for (Map.Entry, FutureTypeAdapter> resolvedAdapterEntry : threadCalls.entrySet()) { + TypeAdapter resolvedAdapter = resolvedAdapterEntry.getValue().delegate; + + // resolvedAdapter might be null if getAdapter(...) threw exception, but caller + // discarded it (instead of rethrowing it) + if (resolvedAdapter != null) { + typeTokenCache.putIfAbsent(resolvedAdapterEntry.getKey(), resolvedAdapter); + } + } + } return candidate; } } throw new IllegalArgumentException("GSON (" + GsonBuildConfig.VERSION + ") cannot handle " + type); } finally { - threadCalls.remove(type); - if (requiresThreadLocalCleanup) { calls.remove(); } @@ -1004,7 +1019,7 @@ public T fromJson(JsonElement json, Type typeOfT) throws JsonSyntaxException } static class FutureTypeAdapter extends TypeAdapter { - private TypeAdapter delegate; + TypeAdapter delegate; public void setDelegate(TypeAdapter typeAdapter) { if (delegate != null) { @@ -1013,18 +1028,21 @@ public void setDelegate(TypeAdapter typeAdapter) { delegate = typeAdapter; } - @Override public T read(JsonReader in) throws IOException { + private TypeAdapter getResolvedDelegate() { + TypeAdapter delegate = this.delegate; if (delegate == null) { - throw new IllegalStateException(); + throw new IllegalStateException("Adapter for type with cyclic dependency has been leaked to " + + "other thread before dependency has been resolved"); } - return delegate.read(in); + return delegate; + } + + @Override public T read(JsonReader in) throws IOException { + return getResolvedDelegate().read(in); } @Override public void write(JsonWriter out, T value) throws IOException { - if (delegate == null) { - throw new IllegalStateException(); - } - delegate.write(out, value); + getResolvedDelegate().write(out, value); } } From 940814550bca24ad66bd07c1d621e3a8e50fa0f7 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Wed, 2 Dec 2020 00:14:56 +0100 Subject: [PATCH 2/7] Improve handling of broken adapters during Gson.getAdapter(...) call --- gson/src/main/java/com/google/gson/Gson.java | 47 +++++++--- .../test/java/com/google/gson/GsonTest.java | 92 +++++++++++++++++++ 2 files changed, 127 insertions(+), 12 deletions(-) diff --git a/gson/src/main/java/com/google/gson/Gson.java b/gson/src/main/java/com/google/gson/Gson.java index 446149c7a5..cbaba3a580 100644 --- a/gson/src/main/java/com/google/gson/Gson.java +++ b/gson/src/main/java/com/google/gson/Gson.java @@ -28,7 +28,8 @@ import java.text.DateFormat; import java.util.ArrayList; import java.util.Collections; -import java.util.HashMap; +import java.util.Iterator; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; @@ -122,8 +123,8 @@ public final class Gson { * lookup would stack overflow. We cheat by returning a proxy type adapter. * The proxy is wired up once the initial adapter has been created. */ - private final ThreadLocal, FutureTypeAdapter>> calls - = new ThreadLocal, FutureTypeAdapter>>(); + private final ThreadLocal, FutureTypeAdapter>> calls + = new ThreadLocal, FutureTypeAdapter>>(); private final ConcurrentMap, TypeAdapter> typeTokenCache = new ConcurrentHashMap, TypeAdapter>(); @@ -437,10 +438,10 @@ public TypeAdapter getAdapter(TypeToken type) { return (TypeAdapter) cached; } - Map, FutureTypeAdapter> threadCalls = calls.get(); + LinkedHashMap, FutureTypeAdapter> threadCalls = calls.get(); boolean requiresThreadLocalCleanup = false; if (threadCalls == null) { - threadCalls = new HashMap, FutureTypeAdapter>(); + threadCalls = new LinkedHashMap, FutureTypeAdapter>(); calls.set(threadCalls); requiresThreadLocalCleanup = true; } @@ -452,6 +453,8 @@ public TypeAdapter getAdapter(TypeToken type) { return resolvedAdapter != null ? resolvedAdapter : ongoingCall; } + int existingAdaptersCount = threadCalls.size(); + boolean foundCandidate = false; try { FutureTypeAdapter call = new FutureTypeAdapter(); threadCalls.put(type, call); @@ -468,14 +471,10 @@ public TypeAdapter getAdapter(TypeToken type) { // See https://github.com/google/gson/issues/625 for (Map.Entry, FutureTypeAdapter> resolvedAdapterEntry : threadCalls.entrySet()) { TypeAdapter resolvedAdapter = resolvedAdapterEntry.getValue().delegate; - - // resolvedAdapter might be null if getAdapter(...) threw exception, but caller - // discarded it (instead of rethrowing it) - if (resolvedAdapter != null) { - typeTokenCache.putIfAbsent(resolvedAdapterEntry.getKey(), resolvedAdapter); - } + typeTokenCache.putIfAbsent(resolvedAdapterEntry.getKey(), resolvedAdapter); } } + foundCandidate = true; return candidate; } } @@ -484,6 +483,22 @@ public TypeAdapter getAdapter(TypeToken type) { if (requiresThreadLocalCleanup) { calls.remove(); } + if (!foundCandidate) { + Iterator> adaptersIterator = threadCalls.values().iterator(); + // Skip existing non-broken adapters + for (; existingAdaptersCount > 0; existingAdaptersCount--) { + adaptersIterator.next(); + } + // Remove this future adapter and all nested ones because they might + // refer to broken adapters + while (adaptersIterator.hasNext()) { + FutureTypeAdapter brokenAdapter = adaptersIterator.next(); + // Mark adapter as broken so user sees useful exception message in + // case TypeAdapterFactory leaks reference to broken adapter + brokenAdapter.markBroken(); + adaptersIterator.remove(); + } + } } } @@ -1019,7 +1034,8 @@ public T fromJson(JsonElement json, Type typeOfT) throws JsonSyntaxException } static class FutureTypeAdapter extends TypeAdapter { - TypeAdapter delegate; + TypeAdapter delegate = null; + private boolean isBroken = false; public void setDelegate(TypeAdapter typeAdapter) { if (delegate != null) { @@ -1028,8 +1044,15 @@ public void setDelegate(TypeAdapter typeAdapter) { delegate = typeAdapter; } + public void markBroken() { + isBroken = true; + } + private TypeAdapter getResolvedDelegate() { TypeAdapter delegate = this.delegate; + if (isBroken) { + throw new IllegalStateException("Broken adapter has been leaked by TypeAdapterFactory"); + } if (delegate == null) { throw new IllegalStateException("Adapter for type with cyclic dependency has been leaked to " + "other thread before dependency has been resolved"); diff --git a/gson/src/test/java/com/google/gson/GsonTest.java b/gson/src/test/java/com/google/gson/GsonTest.java index eec2ec91ca..1cf19d2b29 100644 --- a/gson/src/test/java/com/google/gson/GsonTest.java +++ b/gson/src/test/java/com/google/gson/GsonTest.java @@ -17,6 +17,7 @@ package com.google.gson; import com.google.gson.internal.Excluder; +import com.google.gson.reflect.TypeToken; import com.google.gson.stream.JsonReader; import com.google.gson.stream.JsonWriter; import java.io.IOException; @@ -77,4 +78,95 @@ private static final class TestTypeAdapter extends TypeAdapter { } @Override public Object read(JsonReader in) throws IOException { return null; } } + + /** + * Verify that {@link Gson#getAdapter(TypeToken)} does not put broken adapters + * into {@code typeTokenCache} when caller of nested {@code getAdapter} discards + * exception, e.g.: + * + * Field dependencies: + * ClassA + * -> ClassB1 + * -> ClassC -> ClassB1 + * -> ClassX + * | ClassB2 + * + * Let's assume the factory for ClassX throws an exception. + * 1. Factory for ClassA finds field of type ClassB1 + * 2. Factory for ClassB1 finds field of type ClassC + * 3. Factory for ClassC find fields of type ClassB1 => stores future adapter + * 4. Factory for ClassB1 finds field of type ClassX => ClassX factory throws exception + * 5. Factory for ClassA ignores exception from getAdapter(ClassB1) and tries as alternative getting + * adapter for ClassB2 + * + * Then Gson must not cache adapter for ClassC because it refers to broken adapter + * for ClassB1 (since ClassX throw exception). + */ + public void testGetAdapterDiscardedException() { + Gson gson = new GsonBuilder() + .registerTypeAdapterFactory(new TypeAdapterFactory() { + @Override + public TypeAdapter create(Gson gson, TypeToken type) { + if (type.getRawType() == CustomClassA.class) { + // Factory will throw for CustomClassB1 + try { + gson.getAdapter(CustomClassB1.class); + fail("Expected exception"); + } catch (Exception e) { + } + + return new DummyAdapter(); + } + else if (type.getRawType() == CustomClassB1.class) { + gson.getAdapter(CustomClassC.class); + // Will throw exception + gson.getAdapter(CustomClassX.class); + + throw new AssertionError("Factory should have thrown exception for CustomClassX"); + } + else if (type.getRawType() == CustomClassC.class) { + // Will return future adapter due to cyclic dependency B1 -> C -> B1 + gson.getAdapter(CustomClassB1.class); + return new DummyAdapter(); + } + else if (type.getRawType() == CustomClassX.class) { + // Always throw exception + throw new RuntimeException("test exception"); + } + + throw new AssertionError("Requested adapter for unexpected type: " + type); + } + }) + .create(); + + assertTrue(gson.getAdapter(CustomClassA.class) instanceof DummyAdapter); + // Gson must not have cached broken adapters for CustomClassB1 and CustomClassC + try { + gson.getAdapter(CustomClassB1.class); + fail("Expected exception"); + } catch (Exception e) { + } + try { + gson.getAdapter(CustomClassC.class); + fail("Expected exception"); + } catch (Exception e) { + } + } + + private static class DummyAdapter extends TypeAdapter { + @Override public T read(JsonReader in) throws IOException { + return null; + } + @Override public void write(JsonWriter out, T value) throws IOException { + } + } + + private static class CustomClassA { + } + private static class CustomClassB1 { + } + private static class CustomClassC { + } + private static class CustomClassX { + } } From 1bb129df1bee5e72df56e158e4b8f25a12f37e95 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Sat, 5 Feb 2022 22:29:08 +0100 Subject: [PATCH 3/7] Improve test --- gson/src/main/java/com/google/gson/Gson.java | 8 +++---- .../test/java/com/google/gson/GsonTest.java | 22 ++++++++++++++----- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/gson/src/main/java/com/google/gson/Gson.java b/gson/src/main/java/com/google/gson/Gson.java index 707ba216d7..d591851163 100644 --- a/gson/src/main/java/com/google/gson/Gson.java +++ b/gson/src/main/java/com/google/gson/Gson.java @@ -482,11 +482,11 @@ public TypeAdapter getAdapter(TypeToken type) { } LinkedHashMap, FutureTypeAdapter> threadCalls = calls.get(); - boolean requiresThreadLocalCleanup = false; + boolean isInitialAdapterRequest = false; if (threadCalls == null) { threadCalls = new LinkedHashMap, FutureTypeAdapter>(); calls.set(threadCalls); - requiresThreadLocalCleanup = true; + isInitialAdapterRequest = true; } // the key and value type parameters always agree @@ -507,7 +507,7 @@ public TypeAdapter getAdapter(TypeToken type) { if (candidate != null) { call.setDelegate(candidate); - if (requiresThreadLocalCleanup) { + if (isInitialAdapterRequest) { // Publish resolved adapters to all threads // Can only do this for the initial request because cyclic dependency TypeA -> TypeB -> TypeA // would otherwise publish adapter for TypeB which uses not yet resolved adapter for TypeA @@ -523,7 +523,7 @@ public TypeAdapter getAdapter(TypeToken type) { } throw new IllegalArgumentException("GSON (" + GsonBuildConfig.VERSION + ") cannot handle " + type); } finally { - if (requiresThreadLocalCleanup) { + if (isInitialAdapterRequest) { calls.remove(); } if (!foundCandidate) { diff --git a/gson/src/test/java/com/google/gson/GsonTest.java b/gson/src/test/java/com/google/gson/GsonTest.java index 86491974b9..ef9dfdeb8d 100644 --- a/gson/src/test/java/com/google/gson/GsonTest.java +++ b/gson/src/test/java/com/google/gson/GsonTest.java @@ -16,6 +16,7 @@ package com.google.gson; +import com.google.gson.Gson.FutureTypeAdapter; import com.google.gson.internal.Excluder; import com.google.gson.reflect.TypeToken; import com.google.gson.stream.JsonReader; @@ -175,22 +176,27 @@ public void testNewJsonReader_Custom() throws IOException { * adapter for ClassB2 * * Then Gson must not cache adapter for ClassC because it refers to broken adapter - * for ClassB1 (since ClassX throw exception). + * for ClassB1 (since ClassX threw exception). */ public void testGetAdapterDiscardedException() { + final TypeAdapter alternativeAdapter = new DummyAdapter<>(); + Gson gson = new GsonBuilder() .registerTypeAdapterFactory(new TypeAdapterFactory() { @Override public TypeAdapter create(Gson gson, TypeToken type) { if (type.getRawType() == CustomClassA.class) { - // Factory will throw for CustomClassB1 + // Factory will throw for CustomClassB1; discard exception try { gson.getAdapter(CustomClassB1.class); fail("Expected exception"); } catch (Exception e) { + assertEquals("test exception", e.getMessage()); } - return new DummyAdapter(); + @SuppressWarnings("unchecked") + TypeAdapter adapter = (TypeAdapter) alternativeAdapter; + return adapter; } else if (type.getRawType() == CustomClassB1.class) { gson.getAdapter(CustomClassC.class); @@ -201,7 +207,8 @@ else if (type.getRawType() == CustomClassB1.class) { } else if (type.getRawType() == CustomClassC.class) { // Will return future adapter due to cyclic dependency B1 -> C -> B1 - gson.getAdapter(CustomClassB1.class); + TypeAdapter adapter = gson.getAdapter(CustomClassB1.class); + assertTrue(adapter instanceof FutureTypeAdapter); return new DummyAdapter(); } else if (type.getRawType() == CustomClassX.class) { @@ -214,17 +221,19 @@ else if (type.getRawType() == CustomClassX.class) { }) .create(); - assertTrue(gson.getAdapter(CustomClassA.class) instanceof DummyAdapter); + assertSame(alternativeAdapter, gson.getAdapter(CustomClassA.class)); // Gson must not have cached broken adapters for CustomClassB1 and CustomClassC try { gson.getAdapter(CustomClassB1.class); fail("Expected exception"); } catch (Exception e) { + assertEquals("test exception", e.getMessage()); } try { gson.getAdapter(CustomClassC.class); fail("Expected exception"); } catch (Exception e) { + assertEquals("test exception", e.getMessage()); } } @@ -243,4 +252,5 @@ private static class CustomClassB1 { private static class CustomClassC { } private static class CustomClassX { - }} + } +} From 96e609b76d5fe5b93d4c44121606a029516c4ffe Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Wed, 23 Nov 2022 00:47:27 +0100 Subject: [PATCH 4/7] Slightly improve implementation and extend tests --- gson/src/main/java/com/google/gson/Gson.java | 83 +++++------ .../test/java/com/google/gson/GsonTest.java | 129 +++++++++++++++++- 2 files changed, 167 insertions(+), 45 deletions(-) diff --git a/gson/src/main/java/com/google/gson/Gson.java b/gson/src/main/java/com/google/gson/Gson.java index d591851163..df987a055c 100644 --- a/gson/src/main/java/com/google/gson/Gson.java +++ b/gson/src/main/java/com/google/gson/Gson.java @@ -16,27 +16,6 @@ package com.google.gson; -import java.io.EOFException; -import java.io.IOException; -import java.io.Reader; -import java.io.StringReader; -import java.io.StringWriter; -import java.io.Writer; -import java.lang.reflect.Type; -import java.math.BigDecimal; -import java.math.BigInteger; -import java.text.DateFormat; -import java.util.ArrayList; -import java.util.Collections; -import java.util.Iterator; -import java.util.LinkedHashMap; -import java.util.List; -import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentMap; -import java.util.concurrent.atomic.AtomicLong; -import java.util.concurrent.atomic.AtomicLongArray; - import com.google.gson.internal.ConstructorConstructor; import com.google.gson.internal.Excluder; import com.google.gson.internal.GsonBuildConfig; @@ -60,6 +39,26 @@ import com.google.gson.stream.JsonToken; import com.google.gson.stream.JsonWriter; import com.google.gson.stream.MalformedJsonException; +import java.io.EOFException; +import java.io.IOException; +import java.io.Reader; +import java.io.StringReader; +import java.io.StringWriter; +import java.io.Writer; +import java.lang.reflect.Type; +import java.math.BigDecimal; +import java.math.BigInteger; +import java.text.DateFormat; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Iterator; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicLongArray; /** * This is the main class for using Gson. Gson is typically used by first constructing a @@ -129,8 +128,8 @@ public final class Gson { * lookup would stack overflow. We cheat by returning a proxy type adapter. * The proxy is wired up once the initial adapter has been created. */ - private final ThreadLocal, FutureTypeAdapter>> calls - = new ThreadLocal, FutureTypeAdapter>>(); + // Uses LinkedHashMap because iteration order is important, see getAdapter() implementation below + private final ThreadLocal, TypeAdapter>> calls = new ThreadLocal<>(); private final ConcurrentMap, TypeAdapter> typeTokenCache = new ConcurrentHashMap, TypeAdapter>(); @@ -481,40 +480,40 @@ public TypeAdapter getAdapter(TypeToken type) { return (TypeAdapter) cached; } - LinkedHashMap, FutureTypeAdapter> threadCalls = calls.get(); + LinkedHashMap, TypeAdapter> threadCalls = calls.get(); boolean isInitialAdapterRequest = false; if (threadCalls == null) { - threadCalls = new LinkedHashMap, FutureTypeAdapter>(); + threadCalls = new LinkedHashMap<>(); calls.set(threadCalls); isInitialAdapterRequest = true; } // the key and value type parameters always agree - FutureTypeAdapter ongoingCall = (FutureTypeAdapter) threadCalls.get(type); + TypeAdapter ongoingCall = (TypeAdapter) threadCalls.get(type); if (ongoingCall != null) { - TypeAdapter resolvedAdapter = ongoingCall.delegate; - return resolvedAdapter != null ? resolvedAdapter : ongoingCall; + return ongoingCall; } int existingAdaptersCount = threadCalls.size(); boolean foundCandidate = false; try { - FutureTypeAdapter call = new FutureTypeAdapter(); + FutureTypeAdapter call = new FutureTypeAdapter<>(); threadCalls.put(type, call); for (TypeAdapterFactory factory : factories) { TypeAdapter candidate = factory.create(this, type); if (candidate != null) { call.setDelegate(candidate); + // Replace future adapter with actual adapter + threadCalls.put(type, candidate); if (isInitialAdapterRequest) { // Publish resolved adapters to all threads // Can only do this for the initial request because cyclic dependency TypeA -> TypeB -> TypeA // would otherwise publish adapter for TypeB which uses not yet resolved adapter for TypeA // See https://github.com/google/gson/issues/625 - for (Map.Entry, FutureTypeAdapter> resolvedAdapterEntry : threadCalls.entrySet()) { - TypeAdapter resolvedAdapter = resolvedAdapterEntry.getValue().delegate; - typeTokenCache.putIfAbsent(resolvedAdapterEntry.getKey(), resolvedAdapter); + for (Map.Entry, TypeAdapter> resolvedAdapterEntry : threadCalls.entrySet()) { + typeTokenCache.putIfAbsent(resolvedAdapterEntry.getKey(), resolvedAdapterEntry.getValue()); } } foundCandidate = true; @@ -527,7 +526,7 @@ public TypeAdapter getAdapter(TypeToken type) { calls.remove(); } if (!foundCandidate) { - Iterator> adaptersIterator = threadCalls.values().iterator(); + Iterator> adaptersIterator = threadCalls.values().iterator(); // Skip existing non-broken adapters for (; existingAdaptersCount > 0; existingAdaptersCount--) { adaptersIterator.next(); @@ -535,10 +534,12 @@ public TypeAdapter getAdapter(TypeToken type) { // Remove this future adapter and all nested ones because they might // refer to broken adapters while (adaptersIterator.hasNext()) { - FutureTypeAdapter brokenAdapter = adaptersIterator.next(); - // Mark adapter as broken so user sees useful exception message in - // case TypeAdapterFactory leaks reference to broken adapter - brokenAdapter.markBroken(); + TypeAdapter brokenAdapter = adaptersIterator.next(); + if (brokenAdapter instanceof FutureTypeAdapter) { + // Mark adapter as broken so user sees useful exception message in + // case TypeAdapterFactory leaks reference to broken adapter + ((FutureTypeAdapter) brokenAdapter).markBroken(); + } adaptersIterator.remove(); } } @@ -1093,7 +1094,7 @@ public T fromJson(JsonElement json, Type typeOfT) throws JsonSyntaxException } static class FutureTypeAdapter extends TypeAdapter { - TypeAdapter delegate = null; + private TypeAdapter delegate = null; private boolean isBroken = false; public void setDelegate(TypeAdapter typeAdapter) { @@ -1113,8 +1114,10 @@ private TypeAdapter getResolvedDelegate() { throw new IllegalStateException("Broken adapter has been leaked by TypeAdapterFactory"); } if (delegate == null) { - throw new IllegalStateException("Adapter for type with cyclic dependency has been leaked to " - + "other thread before dependency has been resolved"); + // Can occur when adapter is leaked to other thread or when adapter is used for (de-)serialization + // directly within the TypeAdapterFactory which requested it + throw new IllegalStateException("Adapter for type with cyclic dependency has been used" + + " before dependency has been resolved"); } return delegate; } diff --git a/gson/src/test/java/com/google/gson/GsonTest.java b/gson/src/test/java/com/google/gson/GsonTest.java index ef9dfdeb8d..7ddcff3826 100644 --- a/gson/src/test/java/com/google/gson/GsonTest.java +++ b/gson/src/test/java/com/google/gson/GsonTest.java @@ -30,6 +30,8 @@ import java.text.DateFormat; import java.util.ArrayList; import java.util.HashMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicReference; import junit.framework.TestCase; /** @@ -156,7 +158,7 @@ public void testNewJsonReader_Custom() throws IOException { } /** - * Verify that {@link Gson#getAdapter(TypeToken)} does not put broken adapters + * Verifies that {@link Gson#getAdapter(TypeToken)} does not put broken adapters * into {@code typeTokenCache} when caller of nested {@code getAdapter} discards * exception, e.g.: * @@ -178,8 +180,9 @@ public void testNewJsonReader_Custom() throws IOException { * Then Gson must not cache adapter for ClassC because it refers to broken adapter * for ClassB1 (since ClassX threw exception). */ - public void testGetAdapterDiscardedException() { + public void testGetAdapterDiscardedException() throws Exception { final TypeAdapter alternativeAdapter = new DummyAdapter<>(); + final AtomicReference> leakedAdapter = new AtomicReference<>(); Gson gson = new GsonBuilder() .registerTypeAdapterFactory(new TypeAdapterFactory() { @@ -209,6 +212,8 @@ else if (type.getRawType() == CustomClassC.class) { // Will return future adapter due to cyclic dependency B1 -> C -> B1 TypeAdapter adapter = gson.getAdapter(CustomClassB1.class); assertTrue(adapter instanceof FutureTypeAdapter); + // Pretend this factory somehow leaks this FutureTypeAdapter + leakedAdapter.set(adapter); return new DummyAdapter(); } else if (type.getRawType() == CustomClassX.class) { @@ -235,13 +240,127 @@ else if (type.getRawType() == CustomClassX.class) { } catch (Exception e) { assertEquals("test exception", e.getMessage()); } + + // Leaked adapter should have been marked as "broken" + try { + leakedAdapter.get().fromJson("{}"); + fail("Expected exception"); + } catch (IllegalStateException e) { + assertEquals("Broken adapter has been leaked by TypeAdapterFactory", e.getMessage()); + } } - private static class DummyAdapter extends TypeAdapter { - @Override public T read(JsonReader in) throws IOException { - return null; + /** + * Verifies that two threads calling {@link Gson#getAdapter(TypeToken)} do not see the + * same unresolved {@link FutureTypeAdapter} instance, since that would not be thread-safe. + * + * This test constructs the cyclic dependency CustomClassA -> CustomClassB1 -> CustomClassA + * and lets one thread wait after the adapter for CustomClassB1 has been obtained (which still + * contains the nested unresolved FutureTypeAdapter for CustomClassA). + */ + public void testGetAdapterFutureAdapterConcurrency() throws Exception { + /** + * Adapter which wraps another adapter. Can be imagined as a simplified version of the + * ReflectiveTypeAdapterFactory$Adapter. + */ + class WrappingAdapter extends TypeAdapter { + final TypeAdapter wrapped; + int callCount = 0; + + WrappingAdapter(TypeAdapter wrapped) { + this.wrapped = wrapped; + } + + @Override public void write(JsonWriter out, T value) throws IOException { + // Due to how this test is set up there is infinite recursion, therefore + // need to track how deeply nested this call is + if (callCount == 0) { + callCount++; + out.beginArray(); + wrapped.write(out, null); + out.endArray(); + } else { + out.value("wrapped-nested"); + } + } + + @Override public T read(JsonReader in) throws IOException { + throw new AssertionError("not needed for this test"); + } } + + final CountDownLatch isThreadWaiting = new CountDownLatch(1); + final CountDownLatch canThreadProceed = new CountDownLatch(1); + + final Gson gson = new GsonBuilder() + .registerTypeAdapterFactory(new TypeAdapterFactory() { + // volatile instead of AtomicBoolean is safe here because CountDownLatch prevents + // "true" concurrency + volatile boolean isFirstCaller = true; + + @Override + public TypeAdapter create(Gson gson, TypeToken type) { + Class raw = type.getRawType(); + + if (raw == CustomClassA.class) { + // Retrieves a WrappingAdapter containing a nested FutureAdapter for CustomClassA + TypeAdapter adapter = gson.getAdapter(CustomClassB1.class); + + // Let thread wait so the FutureAdapter for CustomClassA nested in the adapter + // for CustomClassB1 has not been resolved yet + if (isFirstCaller) { + isFirstCaller = false; + isThreadWaiting.countDown(); + + try { + canThreadProceed.await(); + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + } + + return new WrappingAdapter<>(adapter); + } + else if (raw == CustomClassB1.class) { + TypeAdapter adapter = gson.getAdapter(CustomClassA.class); + assertTrue(adapter instanceof FutureTypeAdapter); + return new WrappingAdapter<>(adapter); + } + else { + throw new AssertionError("Adapter for unexpected type requested: " + raw); + } + } + }) + .create(); + + final AtomicReference> otherThreadAdapter = new AtomicReference<>(); + Thread thread = new Thread() { + @Override + public void run() { + otherThreadAdapter.set(gson.getAdapter(CustomClassA.class)); + } + }; + thread.start(); + + // Wait until other thread has obtained FutureAdapter + isThreadWaiting.await(); + TypeAdapter adapter = gson.getAdapter(CustomClassA.class); + // Should not fail due to referring to unresolved FutureTypeAdapter + assertEquals("[[\"wrapped-nested\"]]", adapter.toJson(null)); + + // Let other thread proceed and have it resolve its FutureTypeAdapter + canThreadProceed.countDown(); + thread.join(); + assertEquals("[[\"wrapped-nested\"]]", otherThreadAdapter.get().toJson(null)); + } + + private static class DummyAdapter extends TypeAdapter { @Override public void write(JsonWriter out, T value) throws IOException { + throw new AssertionError("not needed for this test"); + } + + @Override public T read(JsonReader in) throws IOException { + throw new AssertionError("not needed for this test"); } } From 29a69697ffe073e466ead12632d7ec9649d4fd31 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Sat, 3 Dec 2022 20:21:23 +0100 Subject: [PATCH 5/7] Simplify getAdapter implementation --- gson/src/main/java/com/google/gson/Gson.java | 118 +++--- .../test/java/com/google/gson/GsonTest.java | 339 ++++++------------ 2 files changed, 167 insertions(+), 290 deletions(-) diff --git a/gson/src/main/java/com/google/gson/Gson.java b/gson/src/main/java/com/google/gson/Gson.java index 74c00134b0..b43132b438 100644 --- a/gson/src/main/java/com/google/gson/Gson.java +++ b/gson/src/main/java/com/google/gson/Gson.java @@ -52,8 +52,7 @@ import java.text.DateFormat; import java.util.ArrayList; import java.util.Collections; -import java.util.Iterator; -import java.util.LinkedHashMap; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; @@ -156,14 +155,18 @@ public final class Gson { private static final String JSON_NON_EXECUTABLE_PREFIX = ")]}'\n"; /** - * This thread local guards against reentrant calls to getAdapter(). In - * certain object graphs, creating an adapter for a type may recursively + * This thread local guards against reentrant calls to {@link #getAdapter(TypeToken)}. + * In certain object graphs, creating an adapter for a type may recursively * require an adapter for the same type! Without intervention, the recursive - * lookup would stack overflow. We cheat by returning a proxy type adapter. - * The proxy is wired up once the initial adapter has been created. + * lookup would stack overflow. We cheat by returning a proxy type adapter, + * {@link FutureTypeAdapter}, which is wired up once the initial adapter has + * been created. + * + *

The map stores the type adapters for ongoing {@code getAdapter} calls, + * with the type token provided to {@code getAdapter} as key and either + * {@code FutureTypeAdapter} or a regular {@code TypeAdapter} as value. */ - // Uses LinkedHashMap because iteration order is important, see getAdapter() implementation below - private final ThreadLocal, TypeAdapter>> calls = new ThreadLocal<>(); + private final ThreadLocal, TypeAdapter>> threadLocalAdapterResults = new ThreadLocal<>(); private final ConcurrentMap, TypeAdapter> typeTokenCache = new ConcurrentHashMap<>(); @@ -524,77 +527,55 @@ public TypeAdapter getAdapter(TypeToken type) { return adapter; } - LinkedHashMap, TypeAdapter> threadCalls = calls.get(); + Map, TypeAdapter> threadCalls = threadLocalAdapterResults.get(); boolean isInitialAdapterRequest = false; if (threadCalls == null) { - threadCalls = new LinkedHashMap<>(); - calls.set(threadCalls); + threadCalls = new HashMap<>(); + threadLocalAdapterResults.set(threadCalls); isInitialAdapterRequest = true; + } else { + // the key and value type parameters always agree + @SuppressWarnings("unchecked") + TypeAdapter ongoingCall = (TypeAdapter) threadCalls.get(type); + if (ongoingCall != null) { + return ongoingCall; + } } - // the key and value type parameters always agree - @SuppressWarnings("unchecked") - TypeAdapter ongoingCall = (TypeAdapter) threadCalls.get(type); - if (ongoingCall != null) { - return ongoingCall; - } - - int existingAdaptersCount = threadCalls.size(); - boolean foundCandidate = false; + TypeAdapter candidate = null; try { FutureTypeAdapter call = new FutureTypeAdapter<>(); threadCalls.put(type, call); for (TypeAdapterFactory factory : factories) { - TypeAdapter candidate = factory.create(this, type); + candidate = factory.create(this, type); if (candidate != null) { call.setDelegate(candidate); // Replace future adapter with actual adapter threadCalls.put(type, candidate); - - if (isInitialAdapterRequest) { - // Publish resolved adapters to all threads - // Can only do this for the initial request because cyclic dependency TypeA -> TypeB -> TypeA - // would otherwise publish adapter for TypeB which uses not yet resolved adapter for TypeA - // See https://github.com/google/gson/issues/625 - for (Map.Entry, TypeAdapter> resolvedAdapterEntry : threadCalls.entrySet()) { - typeTokenCache.putIfAbsent(resolvedAdapterEntry.getKey(), resolvedAdapterEntry.getValue()); - } - - @SuppressWarnings("unchecked") - TypeAdapter actualAdapter = (TypeAdapter) typeTokenCache.get(type); - // Prefer the actual adapter, in case putIfAbsent call above had no effect because other - // thread already concurrently added other adapter instance for the same type - candidate = actualAdapter; - } - foundCandidate = true; - return candidate; + break; } } - throw new IllegalArgumentException("GSON (" + GsonBuildConfig.VERSION + ") cannot handle " + type); } finally { if (isInitialAdapterRequest) { - calls.remove(); - } - if (!foundCandidate) { - Iterator> adaptersIterator = threadCalls.values().iterator(); - // Skip existing non-broken adapters - for (; existingAdaptersCount > 0; existingAdaptersCount--) { - adaptersIterator.next(); - } - // Remove this future adapter and all nested ones because they might - // refer to broken adapters - while (adaptersIterator.hasNext()) { - TypeAdapter brokenAdapter = adaptersIterator.next(); - if (brokenAdapter instanceof FutureTypeAdapter) { - // Mark adapter as broken so user sees useful exception message in - // case TypeAdapterFactory leaks reference to broken adapter - ((FutureTypeAdapter) brokenAdapter).markBroken(); - } - adaptersIterator.remove(); - } + threadLocalAdapterResults.remove(); } } + + if (candidate == null) { + throw new IllegalArgumentException("GSON (" + GsonBuildConfig.VERSION + ") cannot handle " + type); + } + + if (isInitialAdapterRequest) { + /* + * Publish resolved adapters to all threads + * Can only do this for the initial request because cyclic dependency TypeA -> TypeB -> TypeA + * would otherwise publish adapter for TypeB which uses not yet resolved adapter for TypeA + * See https://github.com/google/gson/issues/625 + */ + typeTokenCache.putAll(threadCalls); + } + return candidate; } /** @@ -1350,26 +1331,27 @@ public T fromJson(JsonElement json, TypeToken typeOfT) throws JsonSyntaxE return fromJson(new JsonTreeReader(json), typeOfT); } + /** + * Proxy type adapter for cyclic type graphs. + * + *

Important: Setting the delegate adapter is not thread-safe; instances of + * {@code FutureTypeAdapter} must only be published to other threads after the delegate + * has been set. + * + * @see Gson#threadLocalAdapterResults + */ static class FutureTypeAdapter extends SerializationDelegatingTypeAdapter { private TypeAdapter delegate = null; - private boolean isBroken = false; public void setDelegate(TypeAdapter typeAdapter) { if (delegate != null) { - throw new AssertionError(); + throw new AssertionError("Delegate is already set"); } delegate = typeAdapter; } - public void markBroken() { - isBroken = true; - } - private TypeAdapter delegate() { TypeAdapter delegate = this.delegate; - if (isBroken) { - throw new IllegalStateException("Broken adapter has been leaked by TypeAdapterFactory"); - } if (delegate == null) { // Can occur when adapter is leaked to other thread or when adapter is used for (de-)serialization // directly within the TypeAdapterFactory which requested it diff --git a/gson/src/test/java/com/google/gson/GsonTest.java b/gson/src/test/java/com/google/gson/GsonTest.java index 79e314f8d2..8126678d1d 100644 --- a/gson/src/test/java/com/google/gson/GsonTest.java +++ b/gson/src/test/java/com/google/gson/GsonTest.java @@ -105,6 +105,16 @@ public void testGetAdapter_Null() { } public void testGetAdapter_Concurrency() { + class DummyAdapter extends TypeAdapter { + @Override public void write(JsonWriter out, T value) throws IOException { + throw new AssertionError("not needed for this test"); + } + + @Override public T read(JsonReader in) throws IOException { + throw new AssertionError("not needed for this test"); + } + } + final AtomicInteger adapterInstancesCreated = new AtomicInteger(0); final AtomicReference> threadAdapter = new AtomicReference<>(); final Class requestedType = Number.class; @@ -141,10 +151,114 @@ public void testGetAdapter_Concurrency() { .create(); TypeAdapter adapter = gson.getAdapter(requestedType); - assertTrue(adapter instanceof DummyAdapter); assertEquals(2, adapterInstancesCreated.get()); - // Should be the same adapter instance the concurrent thread received - assertSame(threadAdapter.get(), adapter); + assertTrue(adapter instanceof DummyAdapter); + assertTrue(threadAdapter.get() instanceof DummyAdapter); + } + + /** + * Verifies that two threads calling {@link Gson#getAdapter(TypeToken)} do not see the + * same unresolved {@link FutureTypeAdapter} instance, since that would not be thread-safe. + * + * This test constructs the cyclic dependency {@literal CustomClass1 -> CustomClass2 -> CustomClass1} + * and lets one thread wait after the adapter for CustomClass2 has been obtained (which still + * refers to the nested unresolved FutureTypeAdapter for CustomClass1). + */ + public void testGetAdapter_FutureAdapterConcurrency() throws Exception { + /** + * Adapter which wraps another adapter. Can be imagined as a simplified version of the + * {@code ReflectiveTypeAdapterFactory$Adapter}. + */ + class WrappingAdapter extends TypeAdapter { + final TypeAdapter wrapped; + boolean isFirstCall = true; + + WrappingAdapter(TypeAdapter wrapped) { + this.wrapped = wrapped; + } + + @Override public void write(JsonWriter out, T value) throws IOException { + // Due to how this test is set up there is infinite recursion, therefore + // need to track how deeply nested this call is + if (isFirstCall) { + isFirstCall = false; + out.beginArray(); + wrapped.write(out, null); + out.endArray(); + isFirstCall = true; + } else { + out.value("wrapped-nested"); + } + } + + @Override public T read(JsonReader in) throws IOException { + throw new AssertionError("not needed for this test"); + } + } + + final CountDownLatch isThreadWaiting = new CountDownLatch(1); + final CountDownLatch canThreadProceed = new CountDownLatch(1); + + final Gson gson = new GsonBuilder() + .registerTypeAdapterFactory(new TypeAdapterFactory() { + // volatile instead of AtomicBoolean is safe here because CountDownLatch prevents + // "true" concurrency + volatile boolean isFirstCaller = true; + + @Override + public TypeAdapter create(Gson gson, TypeToken type) { + Class raw = type.getRawType(); + + if (raw == CustomClass1.class) { + // Retrieves a WrappingAdapter containing a nested FutureAdapter for CustomClass1 + TypeAdapter adapter = gson.getAdapter(CustomClass2.class); + + // Let thread wait so the FutureAdapter for CustomClass1 nested in the adapter + // for CustomClass2 is not resolved yet + if (isFirstCaller) { + isFirstCaller = false; + isThreadWaiting.countDown(); + + try { + canThreadProceed.await(); + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + } + + return new WrappingAdapter<>(adapter); + } + else if (raw == CustomClass2.class) { + TypeAdapter adapter = gson.getAdapter(CustomClass1.class); + assertTrue(adapter instanceof FutureTypeAdapter); + return new WrappingAdapter<>(adapter); + } + else { + throw new AssertionError("Adapter for unexpected type requested: " + raw); + } + } + }) + .create(); + + final AtomicReference> otherThreadAdapter = new AtomicReference<>(); + Thread thread = new Thread() { + @Override + public void run() { + otherThreadAdapter.set(gson.getAdapter(CustomClass1.class)); + } + }; + thread.start(); + + // Wait until other thread has obtained FutureAdapter + isThreadWaiting.await(); + TypeAdapter adapter = gson.getAdapter(CustomClass1.class); + // Should not fail due to referring to unresolved FutureTypeAdapter + assertEquals("[[\"wrapped-nested\"]]", adapter.toJson(null)); + + // Let other thread proceed and have it resolve its FutureTypeAdapter + canThreadProceed.countDown(); + thread.join(); + assertEquals("[[\"wrapped-nested\"]]", otherThreadAdapter.get().toJson(null)); } public void testNewJsonWriter_Default() throws IOException { @@ -360,223 +474,4 @@ public CustomClass3() { this(NO_ARG_CONSTRUCTOR_VALUE); } } - - /** - * Verifies that {@link Gson#getAdapter(TypeToken)} does not put broken adapters - * into {@code typeTokenCache} when caller of nested {@code getAdapter} discards - * exception, e.g.: - * - * Field dependencies: - * ClassA - * -> ClassB1 - * -> ClassC -> ClassB1 - * -> ClassX - * | ClassB2 - * - * Let's assume the factory for ClassX throws an exception. - * 1. Factory for ClassA finds field of type ClassB1 - * 2. Factory for ClassB1 finds field of type ClassC - * 3. Factory for ClassC find fields of type ClassB1 => stores future adapter - * 4. Factory for ClassB1 finds field of type ClassX => ClassX factory throws exception - * 5. Factory for ClassA ignores exception from getAdapter(ClassB1) and tries as alternative getting - * adapter for ClassB2 - * - * Then Gson must not cache adapter for ClassC because it refers to broken adapter - * for ClassB1 (since ClassX threw exception). - */ - public void testGetAdapterDiscardedException() throws Exception { - final TypeAdapter alternativeAdapter = new DummyAdapter<>(); - final AtomicReference> leakedAdapter = new AtomicReference<>(); - - Gson gson = new GsonBuilder() - .registerTypeAdapterFactory(new TypeAdapterFactory() { - @Override - public TypeAdapter create(Gson gson, TypeToken type) { - if (type.getRawType() == CustomClassA.class) { - // Factory will throw for CustomClassB1; discard exception - try { - gson.getAdapter(CustomClassB1.class); - fail("Expected exception"); - } catch (Exception e) { - assertEquals("test exception", e.getMessage()); - } - - @SuppressWarnings("unchecked") - TypeAdapter adapter = (TypeAdapter) alternativeAdapter; - return adapter; - } - else if (type.getRawType() == CustomClassB1.class) { - gson.getAdapter(CustomClassC.class); - // Will throw exception - gson.getAdapter(CustomClassX.class); - - throw new AssertionError("Factory should have thrown exception for CustomClassX"); - } - else if (type.getRawType() == CustomClassC.class) { - // Will return future adapter due to cyclic dependency B1 -> C -> B1 - TypeAdapter adapter = gson.getAdapter(CustomClassB1.class); - assertTrue(adapter instanceof FutureTypeAdapter); - // Pretend this factory somehow leaks this FutureTypeAdapter - leakedAdapter.set(adapter); - return new DummyAdapter(); - } - else if (type.getRawType() == CustomClassX.class) { - // Always throw exception - throw new RuntimeException("test exception"); - } - - throw new AssertionError("Requested adapter for unexpected type: " + type); - } - }) - .create(); - - assertSame(alternativeAdapter, gson.getAdapter(CustomClassA.class)); - // Gson must not have cached broken adapters for CustomClassB1 and CustomClassC - try { - gson.getAdapter(CustomClassB1.class); - fail("Expected exception"); - } catch (Exception e) { - assertEquals("test exception", e.getMessage()); - } - try { - gson.getAdapter(CustomClassC.class); - fail("Expected exception"); - } catch (Exception e) { - assertEquals("test exception", e.getMessage()); - } - - // Leaked adapter should have been marked as "broken" - try { - leakedAdapter.get().fromJson("{}"); - fail("Expected exception"); - } catch (IllegalStateException e) { - assertEquals("Broken adapter has been leaked by TypeAdapterFactory", e.getMessage()); - } - } - - /** - * Verifies that two threads calling {@link Gson#getAdapter(TypeToken)} do not see the - * same unresolved {@link FutureTypeAdapter} instance, since that would not be thread-safe. - * - * This test constructs the cyclic dependency CustomClassA -> CustomClassB1 -> CustomClassA - * and lets one thread wait after the adapter for CustomClassB1 has been obtained (which still - * contains the nested unresolved FutureTypeAdapter for CustomClassA). - */ - public void testGetAdapterFutureAdapterConcurrency() throws Exception { - /** - * Adapter which wraps another adapter. Can be imagined as a simplified version of the - * ReflectiveTypeAdapterFactory$Adapter. - */ - class WrappingAdapter extends TypeAdapter { - final TypeAdapter wrapped; - int callCount = 0; - - WrappingAdapter(TypeAdapter wrapped) { - this.wrapped = wrapped; - } - - @Override public void write(JsonWriter out, T value) throws IOException { - // Due to how this test is set up there is infinite recursion, therefore - // need to track how deeply nested this call is - try { - if (callCount++ == 0) { - out.beginArray(); - wrapped.write(out, null); - out.endArray(); - } else { - out.value("wrapped-nested"); - } - } finally { - callCount--; - } - } - - @Override public T read(JsonReader in) throws IOException { - throw new AssertionError("not needed for this test"); - } - } - - final CountDownLatch isThreadWaiting = new CountDownLatch(1); - final CountDownLatch canThreadProceed = new CountDownLatch(1); - - final Gson gson = new GsonBuilder() - .registerTypeAdapterFactory(new TypeAdapterFactory() { - // volatile instead of AtomicBoolean is safe here because CountDownLatch prevents - // "true" concurrency - volatile boolean isFirstCaller = true; - - @Override - public TypeAdapter create(Gson gson, TypeToken type) { - Class raw = type.getRawType(); - - if (raw == CustomClassA.class) { - // Retrieves a WrappingAdapter containing a nested FutureAdapter for CustomClassA - TypeAdapter adapter = gson.getAdapter(CustomClassB1.class); - - // Let thread wait so the FutureAdapter for CustomClassA nested in the adapter - // for CustomClassB1 has not been resolved yet - if (isFirstCaller) { - isFirstCaller = false; - isThreadWaiting.countDown(); - - try { - canThreadProceed.await(); - } catch (InterruptedException e) { - throw new RuntimeException(e); - } - } - - return new WrappingAdapter<>(adapter); - } - else if (raw == CustomClassB1.class) { - TypeAdapter adapter = gson.getAdapter(CustomClassA.class); - assertTrue(adapter instanceof FutureTypeAdapter); - return new WrappingAdapter<>(adapter); - } - else { - throw new AssertionError("Adapter for unexpected type requested: " + raw); - } - } - }) - .create(); - - final AtomicReference> otherThreadAdapter = new AtomicReference<>(); - Thread thread = new Thread() { - @Override - public void run() { - otherThreadAdapter.set(gson.getAdapter(CustomClassA.class)); - } - }; - thread.start(); - - // Wait until other thread has obtained FutureAdapter - isThreadWaiting.await(); - TypeAdapter adapter = gson.getAdapter(CustomClassA.class); - // Should not fail due to referring to unresolved FutureTypeAdapter - assertEquals("[[\"wrapped-nested\"]]", adapter.toJson(null)); - - // Let other thread proceed and have it resolve its FutureTypeAdapter - canThreadProceed.countDown(); - thread.join(); - assertEquals("[[\"wrapped-nested\"]]", otherThreadAdapter.get().toJson(null)); - } - - private static class DummyAdapter extends TypeAdapter { - @Override public void write(JsonWriter out, T value) throws IOException { - throw new AssertionError("not needed for this test"); - } - - @Override public T read(JsonReader in) throws IOException { - throw new AssertionError("not needed for this test"); - } - } - - private static class CustomClassA { - } - private static class CustomClassB1 { - } - private static class CustomClassC { - } - private static class CustomClassX { - } } From 8f2faa6564bad69b3c5692b6e710e33deceb800d Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Sat, 3 Dec 2022 20:25:31 +0100 Subject: [PATCH 6/7] Convert GsonTest to JUnit 4 test --- .../test/java/com/google/gson/GsonTest.java | 26 +++++++++++++++---- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/gson/src/test/java/com/google/gson/GsonTest.java b/gson/src/test/java/com/google/gson/GsonTest.java index 8126678d1d..fd335e49bd 100644 --- a/gson/src/test/java/com/google/gson/GsonTest.java +++ b/gson/src/test/java/com/google/gson/GsonTest.java @@ -16,6 +16,10 @@ package com.google.gson; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + import com.google.gson.Gson.FutureTypeAdapter; import com.google.gson.internal.Excluder; import com.google.gson.reflect.TypeToken; @@ -34,14 +38,14 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; -import junit.framework.TestCase; +import org.junit.Test; /** * Unit tests for {@link Gson}. * * @author Ryan Harter */ -public final class GsonTest extends TestCase { +public final class GsonTest { private static final Excluder CUSTOM_EXCLUDER = Excluder.DEFAULT .excludeFieldsWithoutExposeAnnotation() @@ -56,6 +60,7 @@ public final class GsonTest extends TestCase { private static final ToNumberStrategy CUSTOM_OBJECT_TO_NUMBER_STRATEGY = ToNumberPolicy.DOUBLE; private static final ToNumberStrategy CUSTOM_NUMBER_TO_NUMBER_STRATEGY = ToNumberPolicy.LAZILY_PARSED_NUMBER; + @Test public void testOverridesDefaultExcluder() { Gson gson = new Gson(CUSTOM_EXCLUDER, CUSTOM_FIELD_NAMING_STRATEGY, new HashMap>(), true, false, true, false, @@ -71,6 +76,7 @@ public void testOverridesDefaultExcluder() { assertEquals(false, gson.htmlSafe()); } + @Test public void testClonedTypeAdapterFactoryListsAreIndependent() { Gson original = new Gson(CUSTOM_EXCLUDER, CUSTOM_FIELD_NAMING_STRATEGY, new HashMap>(), true, false, true, false, @@ -94,6 +100,7 @@ private static final class TestTypeAdapter extends TypeAdapter { @Override public Object read(JsonReader in) throws IOException { return null; } } + @Test public void testGetAdapter_Null() { Gson gson = new Gson(); try { @@ -104,6 +111,7 @@ public void testGetAdapter_Null() { } } + @Test public void testGetAdapter_Concurrency() { class DummyAdapter extends TypeAdapter { @Override public void write(JsonWriter out, T value) throws IOException { @@ -164,6 +172,7 @@ class DummyAdapter extends TypeAdapter { * and lets one thread wait after the adapter for CustomClass2 has been obtained (which still * refers to the nested unresolved FutureTypeAdapter for CustomClass1). */ + @Test public void testGetAdapter_FutureAdapterConcurrency() throws Exception { /** * Adapter which wraps another adapter. Can be imagined as a simplified version of the @@ -261,6 +270,7 @@ public void run() { assertEquals("[[\"wrapped-nested\"]]", otherThreadAdapter.get().toJson(null)); } + @Test public void testNewJsonWriter_Default() throws IOException { StringWriter writer = new StringWriter(); JsonWriter jsonWriter = new Gson().newJsonWriter(writer); @@ -283,6 +293,7 @@ public void testNewJsonWriter_Default() throws IOException { assertEquals("{\"\\u003ctest2\":true}", writer.toString()); } + @Test public void testNewJsonWriter_Custom() throws IOException { StringWriter writer = new StringWriter(); JsonWriter jsonWriter = new GsonBuilder() @@ -307,6 +318,7 @@ public void testNewJsonWriter_Custom() throws IOException { assertEquals(")]}'\n{\n \"test\": null,\n \"() { @@ -459,9 +474,9 @@ private static void assertCustomGson(Gson gson) { assertEquals("custom-instance", customClass3.s); } - static class CustomClass1 { } - static class CustomClass2 { } - static class CustomClass3 { + private static class CustomClass1 { } + private static class CustomClass2 { } + private static class CustomClass3 { static final String NO_ARG_CONSTRUCTOR_VALUE = "default instance"; final String s; @@ -470,6 +485,7 @@ public CustomClass3(String s) { this.s = s; } + @SuppressWarnings("unused") // called by Gson public CustomClass3() { this(NO_ARG_CONSTRUCTOR_VALUE); } From a5ba266dfd5ccba7e39cc4041861df65e30a8914 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Sat, 3 Dec 2022 20:57:45 +0100 Subject: [PATCH 7/7] Clarify getAdapter concurrency behavior --- gson/src/main/java/com/google/gson/Gson.java | 13 +++++++++---- .../main/java/com/google/gson/JsonDeserializer.java | 3 +++ .../main/java/com/google/gson/JsonSerializer.java | 3 +++ gson/src/main/java/com/google/gson/TypeAdapter.java | 3 +++ 4 files changed, 18 insertions(+), 4 deletions(-) diff --git a/gson/src/main/java/com/google/gson/Gson.java b/gson/src/main/java/com/google/gson/Gson.java index b43132b438..8a26760fdc 100644 --- a/gson/src/main/java/com/google/gson/Gson.java +++ b/gson/src/main/java/com/google/gson/Gson.java @@ -513,9 +513,14 @@ private static TypeAdapter atomicLongArrayAdapter(final TypeAda } /** - * Returns the type adapter for {@code} type. + * Returns the type adapter for {@code type}. * - * @throws IllegalArgumentException if this GSON cannot serialize and + *

When calling this method concurrently from multiple threads and requesting + * an adapter for the same type this method may return different {@code TypeAdapter} + * instances. However, that should normally not be an issue because {@code TypeAdapter} + * implementations are supposed to be stateless. + * + * @throws IllegalArgumentException if this Gson instance cannot serialize and * deserialize {@code type}. */ public TypeAdapter getAdapter(TypeToken type) { @@ -653,9 +658,9 @@ public TypeAdapter getDelegateAdapter(TypeAdapterFactory skipPast, TypeTo } /** - * Returns the type adapter for {@code} type. + * Returns the type adapter for {@code type}. * - * @throws IllegalArgumentException if this GSON cannot serialize and + * @throws IllegalArgumentException if this Gson instance cannot serialize and * deserialize {@code type}. */ public TypeAdapter getAdapter(Class type) { diff --git a/gson/src/main/java/com/google/gson/JsonDeserializer.java b/gson/src/main/java/com/google/gson/JsonDeserializer.java index 6462d45c0b..cef519462a 100644 --- a/gson/src/main/java/com/google/gson/JsonDeserializer.java +++ b/gson/src/main/java/com/google/gson/JsonDeserializer.java @@ -63,6 +63,9 @@ * Gson gson = new GsonBuilder().registerTypeAdapter(Id.class, new IdDeserializer()).create(); * * + *

Deserializers should be stateless and thread-safe, otherwise the thread-safety + * guarantees of {@link Gson} might not apply. + * *

New applications should prefer {@link TypeAdapter}, whose streaming API * is more efficient than this interface's tree API. * diff --git a/gson/src/main/java/com/google/gson/JsonSerializer.java b/gson/src/main/java/com/google/gson/JsonSerializer.java index 2bdb8fb28c..01d62723b8 100644 --- a/gson/src/main/java/com/google/gson/JsonSerializer.java +++ b/gson/src/main/java/com/google/gson/JsonSerializer.java @@ -60,6 +60,9 @@ * Gson gson = new GsonBuilder().registerTypeAdapter(Id.class, new IdSerializer()).create(); * * + *

Serializers should be stateless and thread-safe, otherwise the thread-safety + * guarantees of {@link Gson} might not apply. + * *

New applications should prefer {@link TypeAdapter}, whose streaming API * is more efficient than this interface's tree API. * diff --git a/gson/src/main/java/com/google/gson/TypeAdapter.java b/gson/src/main/java/com/google/gson/TypeAdapter.java index 98e1668a3f..5fdea225a5 100644 --- a/gson/src/main/java/com/google/gson/TypeAdapter.java +++ b/gson/src/main/java/com/google/gson/TypeAdapter.java @@ -81,6 +81,9 @@ * when writing to a JSON object) will be omitted automatically. In either case * your type adapter must handle null. * + *

Type adapters should be stateless and thread-safe, otherwise the thread-safety + * guarantees of {@link Gson} might not apply. + * *

To use a custom type adapter with Gson, you must register it with a * {@link GsonBuilder}: 

   {@code
  *