Improve handling of broken adapters during Gson.getAdapter(...) call

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<Map<TypeToken<?>, FutureTypeAdapter<?>>> calls
-      = new ThreadLocal<Map<TypeToken<?>, FutureTypeAdapter<?>>>();
+  private final ThreadLocal<LinkedHashMap<TypeToken<?>, FutureTypeAdapter<?>>> calls
+      = new ThreadLocal<LinkedHashMap<TypeToken<?>, FutureTypeAdapter<?>>>();
 
   private final ConcurrentMap<TypeToken<?>, TypeAdapter<?>> typeTokenCache = new ConcurrentHashMap<TypeToken<?>, TypeAdapter<?>>();
 
@@ -437,10 +438,10 @@ public <T> TypeAdapter<T> getAdapter(TypeToken<T> type) {
       return (TypeAdapter<T>) cached;
     }
 
-    Map<TypeToken<?>, FutureTypeAdapter<?>> threadCalls = calls.get();
+    LinkedHashMap<TypeToken<?>, FutureTypeAdapter<?>> threadCalls = calls.get();
     boolean requiresThreadLocalCleanup = false;
     if (threadCalls == null) {
-      threadCalls = new HashMap<TypeToken<?>, FutureTypeAdapter<?>>();
+      threadCalls = new LinkedHashMap<TypeToken<?>, FutureTypeAdapter<?>>();
       calls.set(threadCalls);
       requiresThreadLocalCleanup = true;
     }
@@ -452,6 +453,8 @@ public <T> TypeAdapter<T> getAdapter(TypeToken<T> type) {
       return resolvedAdapter != null ? resolvedAdapter : ongoingCall;
     }
 
+    int existingAdaptersCount = threadCalls.size();
+    boolean foundCandidate = false;
     try {
       FutureTypeAdapter<T> call = new FutureTypeAdapter<T>();
       threadCalls.put(type, call);
@@ -468,14 +471,10 @@ public <T> TypeAdapter<T> getAdapter(TypeToken<T> type) {
             // See https://github.com/google/gson/issues/625
             for (Map.Entry<TypeToken<?>, 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 <T> TypeAdapter<T> getAdapter(TypeToken<T> type) {
       if (requiresThreadLocalCleanup) {
         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();
+        }
+      }
     }
   }
 
@@ -1019,7 +1034,8 @@ public <T> T fromJson(JsonElement json, Type typeOfT) throws JsonSyntaxException
   }
 
   static class FutureTypeAdapter<T> extends TypeAdapter<T> {
-    TypeAdapter<T> delegate;
+    TypeAdapter<T> delegate = null;
+    private boolean isBroken = false;
 
     public void setDelegate(TypeAdapter<T> typeAdapter) {
       if (delegate != null) {
@@ -1028,8 +1044,15 @@ public void setDelegate(TypeAdapter<T> typeAdapter) {
       delegate = typeAdapter;
     }
 
+    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("Adapter for type with cyclic dependency has been leaked to "
             + "other thread before dependency has been resolved");

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<Object> {
     }
     @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 <T> TypeAdapter<T> create(Gson gson, TypeToken<T> 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<T>();
+          }
+          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<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();
+
+    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<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 {
+  }
 }