Fix non-threadsafe creation of adapter for type with cyclic dependency

gson/src/main/java/com/google/gson/Gson.java

@@ -28,10 +28,12 @@
 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;
+import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.atomic.AtomicLongArray;
 
@@ -127,10 +129,10 @@ 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<Map<TypeToken<?>, FutureTypeAdapter<?>>> calls
-      = new ThreadLocal<Map<TypeToken<?>, FutureTypeAdapter<?>>>();
+  private final ThreadLocal<LinkedHashMap<TypeToken<?>, FutureTypeAdapter<?>>> calls
+      = new ThreadLocal<LinkedHashMap<TypeToken<?>, FutureTypeAdapter<?>>>();
 
-  private final Map<TypeToken<?>, TypeAdapter<?>> typeTokenCache = new ConcurrentHashMap<TypeToken<?>, TypeAdapter<?>>();
+  private final ConcurrentMap<TypeToken<?>, TypeAdapter<?>> typeTokenCache = new ConcurrentHashMap<TypeToken<?>, TypeAdapter<?>>();
 
   private final ConstructorConstructor constructorConstructor;
   private final JsonAdapterAnnotationTypeAdapterFactory jsonAdapterFactory;
@@ -479,20 +481,23 @@ public <T> TypeAdapter<T> getAdapter(TypeToken<T> type) {
       return (TypeAdapter<T>) cached;
     }
 
-    Map<TypeToken<?>, FutureTypeAdapter<?>> threadCalls = calls.get();
-    boolean requiresThreadLocalCleanup = false;
+    LinkedHashMap<TypeToken<?>, FutureTypeAdapter<?>> threadCalls = calls.get();
+    boolean isInitialAdapterRequest = false;
     if (threadCalls == null) {
-      threadCalls = new HashMap<TypeToken<?>, FutureTypeAdapter<?>>();
+      threadCalls = new LinkedHashMap<TypeToken<?>, FutureTypeAdapter<?>>();
       calls.set(threadCalls);
-      requiresThreadLocalCleanup = true;
+      isInitialAdapterRequest = true;
     }
 
     // the key and value type parameters always agree
     FutureTypeAdapter<T> ongoingCall = (FutureTypeAdapter<T>) threadCalls.get(type);
     if (ongoingCall != null) {
-      return ongoingCall;
+      TypeAdapter<T> resolvedAdapter = ongoingCall.delegate;
+      return resolvedAdapter != null ? resolvedAdapter : ongoingCall;
     }
 
+    int existingAdaptersCount = threadCalls.size();
+    boolean foundCandidate = false;
     try {
       FutureTypeAdapter<T> call = new FutureTypeAdapter<T>();
       threadCalls.put(type, call);
@@ -501,17 +506,42 @@ public <T> TypeAdapter<T> getAdapter(TypeToken<T> type) {
         TypeAdapter<T> candidate = factory.create(this, type);
         if (candidate != null) {
           call.setDelegate(candidate);
-          typeTokenCache.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<TypeToken<?>, FutureTypeAdapter<?>> resolvedAdapterEntry : threadCalls.entrySet()) {
+              TypeAdapter<?> resolvedAdapter = resolvedAdapterEntry.getValue().delegate;
+              typeTokenCache.putIfAbsent(resolvedAdapterEntry.getKey(), resolvedAdapter);
+            }
+          }
+          foundCandidate = true;
           return candidate;
         }
       }
       throw new IllegalArgumentException("GSON (" + GsonBuildConfig.VERSION + ") cannot handle " + type);
     } finally {
-      threadCalls.remove(type);
-
-      if (requiresThreadLocalCleanup) {
+      if (isInitialAdapterRequest) {
         calls.remove();
       }
+      if (!foundCandidate) {
+        Iterator<FutureTypeAdapter<?>> 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();
+        }
+      }
     }
   }
 
@@ -1063,7 +1093,8 @@ public <T> T fromJson(JsonElement json, Type typeOfT) throws JsonSyntaxException
   }
 
   static class FutureTypeAdapter<T> extends TypeAdapter<T> {
-    private TypeAdapter<T> delegate;
+    TypeAdapter<T> delegate = null;
+    private boolean isBroken = false;
 
     public void setDelegate(TypeAdapter<T> typeAdapter) {
       if (delegate != null) {
@@ -1072,18 +1103,28 @@ public void setDelegate(TypeAdapter<T> typeAdapter) {
       delegate = typeAdapter;
     }
 
-    @Override public T read(JsonReader in) throws IOException {
+    public void markBroken() {
+      isBroken = true;
+    }
+
+    private TypeAdapter<T> getResolvedDelegate() {
+      TypeAdapter<T> delegate = this.delegate;
+      if (isBroken) {
+        throw new IllegalStateException("Broken adapter has been leaked by TypeAdapterFactory");
+      }
       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);
     }
   }
 

gson/src/test/java/com/google/gson/GsonTest.java

@@ -16,7 +16,9 @@
 
 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;
 import com.google.gson.stream.JsonWriter;
 import com.google.gson.stream.MalformedJsonException;
@@ -152,4 +154,103 @@ public void testNewJsonReader_Custom() throws IOException {
     assertEquals("test", jsonReader.nextString());
     jsonReader.close();
   }
+
+  /**
+   * 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 threw exception).
+   */
+  public void testGetAdapterDiscardedException() {
+    final TypeAdapter<?> alternativeAdapter = new DummyAdapter<>();
+
+    Gson gson = new GsonBuilder()
+      .registerTypeAdapterFactory(new TypeAdapterFactory() {
+        @Override
+        public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> 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<T> adapter = (TypeAdapter<T>) 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);
+            return new DummyAdapter<T>();
+          }
+          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());
+    }
+  }
+
+  private static class DummyAdapter<T> extends TypeAdapter<T> {
+    @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 {
+  }
 }