From 2c7990561759c839c310dba44d3bd786ef5bfa33 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Wed, 2 Dec 2020 00:14:56 +0100 Subject: [PATCH] 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 92e7bc3d1f..1a78ff658c 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/764 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..627badae15 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 alternatively 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 { + } }