Fix a race condition on deferred adapters

This changes how we lookup and cache adapters. Previously we were pretty
optimistic about putting adapters in the cache; these adapters could have
depended upon stubs that were incomplete.

Now we're a lot more careful: we only put adapters in the cache if the
root object that triggered a set of recursive calls was constructed
successfully.

Closes: https://github.com/square/moshi/issues/679

moshi/src/main/java/com/squareup/moshi/Moshi.java

@@ -19,9 +19,12 @@ import com.squareup.moshi.internal.Util;
 import java.io.IOException;
 import java.lang.annotation.Annotation;
 import java.lang.reflect.Type;
+import java.util.ArrayDeque;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.Deque;
+import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
@@ -48,7 +51,7 @@ public final class Moshi {
   }
 
   private final List<JsonAdapter.Factory> factories;
-  private final ThreadLocal<List<DeferredAdapter<?>>> reentrantCalls = new ThreadLocal<>();
+  private final ThreadLocal<LookupChain> lookupChainThreadLocal = new ThreadLocal<>();
   private final Map<Object, JsonAdapter<?>> adapterCache = new LinkedHashMap<>();
 
   Moshi(Builder builder) {
@@ -118,52 +121,35 @@ public final class Moshi {
       if (result != null) return (JsonAdapter<T>) result;
     }
 
-    // Short-circuit if this is a reentrant call.
+    LookupChain lookupChain = lookupChainThreadLocal.get();
-    List<DeferredAdapter<?>> deferredAdapters = reentrantCalls.get();
+    if (lookupChain == null) {
-    if (deferredAdapters != null) {
+      lookupChain = new LookupChain();
-      for (int i = 0, size = deferredAdapters.size(); i < size; i++) {
+      lookupChainThreadLocal.set(lookupChain);
-        DeferredAdapter<?> deferredAdapter = deferredAdapters.get(i);
-        if (deferredAdapter.cacheKey.equals(cacheKey)) {
-          return (JsonAdapter<T>) deferredAdapter;
-        }
-      }
-    } else {
-      deferredAdapters = new ArrayList<>();
-      reentrantCalls.set(deferredAdapters);
     }
 
-    // Prepare for re-entrant calls, then ask each factory to create a type adapter.
+    boolean success = false;
-    DeferredAdapter<T> deferredAdapter = new DeferredAdapter<>(type, fieldName, cacheKey);
+    JsonAdapter<T> adapterFromCall = lookupChain.push(type, fieldName, cacheKey);
-
-    deferredAdapters.add(deferredAdapter);
-    int lastIndex = deferredAdapters.size() - 1;
     try {
+      if (adapterFromCall != null) return adapterFromCall;
+
+      // Ask each factory to create the JSON adapter.
       for (int i = 0, size = factories.size(); i < size; i++) {
         JsonAdapter<T> result = (JsonAdapter<T>) factories.get(i).create(type, annotations, this);
-        if (result != null) {
+        if (result == null) continue;
-          deferredAdapter.ready(result);
-          synchronized (adapterCache) {
-            adapterCache.put(cacheKey, result);
-          }
-          // Remove the type or field name only when we succeed in creating the adapter,
-          // so we have the full stack  at the top level.
-          deferredAdapters.remove(lastIndex);
-          return result;
-        }
-      }
-    } catch (IllegalArgumentException e) {
-      if (lastIndex == 0) { // Rewrite the exception at the top level.
-        e = errorWithFields(deferredAdapters, e);
-      }
-      throw e;
-    } finally {
-      if (lastIndex == 0) {
-        reentrantCalls.remove();
-      }
-    }
 
-    throw new IllegalArgumentException(
+        // Success! Notify the LookupChain so it is cached and can be used by re-entrant calls.
-        "No JsonAdapter for " + typeAnnotatedWithAnnotations(type, annotations));
+        lookupChain.adapterFound(result);
+        success = true;
+        return result;
+      }
+
+      throw new IllegalArgumentException(
+          "No JsonAdapter for " + typeAnnotatedWithAnnotations(type, annotations));
+    } catch (IllegalArgumentException e) {
+      throw lookupChain.exceptionWithLookupStack(e);
+    } finally {
+      lookupChain.pop(success);
+    }
   }
 
   @CheckReturnValue
@@ -200,27 +186,6 @@ public final class Moshi {
     return Arrays.asList(type, annotations);
   }
 
-  static IllegalArgumentException errorWithFields(List<DeferredAdapter<?>> typesAndFieldNames,
-      IllegalArgumentException e) {
-    int size = typesAndFieldNames.size();
-    if (size == 1 && typesAndFieldNames.get(0).fieldName == null) {
-      return e;
-    }
-    StringBuilder errorMessageBuilder = new StringBuilder(e.getMessage());
-    for (int i = size - 1; i >= 0; i--) {
-      DeferredAdapter<?> deferredAdapter = typesAndFieldNames.get(i);
-      errorMessageBuilder
-          .append("\nfor ")
-          .append(deferredAdapter.type);
-      if (deferredAdapter.fieldName != null) {
-        errorMessageBuilder
-            .append(' ')
-            .append(deferredAdapter.fieldName);
-      }
-    }
-    return new IllegalArgumentException(errorMessageBuilder.toString(), e);
-  }
-
   public static final class Builder {
     final List<JsonAdapter.Factory> factories = new ArrayList<>();
 
@@ -283,38 +248,132 @@ public final class Moshi {
   }
 
   /**
-   * Sometimes a type adapter factory depends on its own product; either directly or indirectly.
+   * A possibly-reentrant chain of lookups a JSON adapter.
-   * To make this work, we offer this type adapter stub while the final adapter is being computed.
-   * When it is ready, we wire this to delegate to that finished adapter.
    *
-   * <p>Typically this is necessary in self-referential object models, such as an {@code Employee}
+   * <p>We keep track of the current stack of lookups: we may start by looking up the JSON adapter
-   * class that has a {@code List<Employee>} field for an organization's management hierarchy.
+   * for Employee, re-enter looking for the JSON adapter of HomeAddress, and re-enter again looking
+   * up the JSON adapter of PostalCode. If any of these lookups fail we can provide a stack trace
+   * with all of the lookups.
+   *
+   * <p>Sometimes a JSON adapter factory depends on its own product; either directly or indirectly.
+   * To make this work, we offer a JSON adapter stub while the final adapter is being computed.
+   * When it is ready, we wire the stub to that finished adapter. This is necessary in
+   * self-referential object models, such as an {@code Employee} class that has a {@code
+   * List<Employee>} field for an organization's management hierarchy.
+   *
+   * <p>This class defers putting any JSON adapters in the cache until the topmost JSON adapter has
+   * successfully been computed. That way we don't pollute the cache with incomplete stubs, or
+   * adapters that may transitively depend on incomplete stubs.
    */
-  private static final class DeferredAdapter<T> extends JsonAdapter<T> {
+  final class LookupChain {
+    final List<Lookup<?>> callLookups = new ArrayList<>();
+    final Deque<Lookup<?>> stack = new ArrayDeque<>();
+    boolean exceptionAnnotated;
+
+    /**
+     * Returns a JSON adapter that was already created for this call, or null if this is the first
+     * time in this call that the cache key has been requested in this call. This may return a
+     * lookup that isn't yet ready if this lookup is reentrant.
+     */
+    <T> JsonAdapter<T> push(Type type, @Nullable String fieldName, Object cacheKey) {
+      // Try to find a lookup with the same key for the same call.
+      for (int i = 0, size = callLookups.size(); i < size; i++) {
+        Lookup<?> lookup = callLookups.get(i);
+        if (lookup.cacheKey.equals(cacheKey)) {
+          Lookup<T> hit = (Lookup<T>) lookup;
+          stack.add(hit);
+          return hit.adapter != null ? hit.adapter : hit;
+        }
+      }
+
+      // We might need to know about this cache key later in this call. Prepare for that.
+      Lookup<Object> lookup = new Lookup<>(type, fieldName, cacheKey);
+      callLookups.add(lookup);
+      stack.add(lookup);
+      return null;
+    }
+
+    /** Sets the adapter result of the current lookup. */
+    <T> void adapterFound(JsonAdapter<T> result) {
+      Lookup<T> currentLookup = (Lookup<T>) stack.getLast();
+      currentLookup.adapter = result;
+    }
+
+    /**
+     * Completes the current lookup by removing a stack frame.
+     *
+     * @param success true if the adapter cache should be populated if this is the topmost lookup.
+     */
+    void pop(boolean success) {
+      stack.removeLast();
+      if (!stack.isEmpty()) return;
+
+      lookupChainThreadLocal.remove();
+
+      if (success) {
+        synchronized (adapterCache) {
+          for (int i = 0, size = callLookups.size(); i < size; i++) {
+            Lookup<?> lookup = callLookups.get(i);
+            JsonAdapter<?> replaced = adapterCache.put(lookup.cacheKey, lookup.adapter);
+            if (replaced != null) {
+              ((Lookup<Object>) lookup).adapter = (JsonAdapter<Object>) replaced;
+              adapterCache.put(lookup.cacheKey, replaced);
+            }
+          }
+        }
+      }
+    }
+
+    IllegalArgumentException exceptionWithLookupStack(IllegalArgumentException e) {
+      // Don't add the lookup stack to more than one exception; the deepest is sufficient.
+      if (exceptionAnnotated) return e;
+      exceptionAnnotated = true;
+
+      int size = stack.size();
+      if (size == 1 && stack.getFirst().fieldName == null) return e;
+
+      StringBuilder errorMessageBuilder = new StringBuilder(e.getMessage());
+      for (Iterator<Lookup<?>> i = stack.descendingIterator(); i.hasNext(); ) {
+        Lookup<?> lookup = i.next();
+        errorMessageBuilder
+            .append("\nfor ")
+            .append(lookup.type);
+        if (lookup.fieldName != null) {
+          errorMessageBuilder
+              .append(' ')
+              .append(lookup.fieldName);
+        }
+      }
+
+      return new IllegalArgumentException(errorMessageBuilder.toString(), e);
+    }
+  }
+
+  /** This class implements {@code JsonAdapter} so it can be used as a stub for re-entrant calls. */
+  static final class Lookup<T> extends JsonAdapter<T> {
     final Type type;
     final @Nullable String fieldName;
-    @Nullable Object cacheKey;
+    final Object cacheKey;
-    private @Nullable JsonAdapter<T> delegate;
+    @Nullable JsonAdapter<T> adapter;
 
-    DeferredAdapter(Type type, @Nullable String fieldName, Object cacheKey) {
+    Lookup(Type type, @Nullable String fieldName, Object cacheKey) {
       this.type = type;
       this.fieldName = fieldName;
       this.cacheKey = cacheKey;
     }
 
-    void ready(JsonAdapter<T> delegate) {
-      this.delegate = delegate;
-      this.cacheKey = null;
-    }
-
     @Override public T fromJson(JsonReader reader) throws IOException {
-      if (delegate == null) throw new IllegalStateException("Type adapter isn't ready");
+      if (adapter == null) throw new IllegalStateException("JsonAdapter isn't ready");
-      return delegate.fromJson(reader);
+      return adapter.fromJson(reader);
     }
 
     @Override public void toJson(JsonWriter writer, T value) throws IOException {
-      if (delegate == null) throw new IllegalStateException("Type adapter isn't ready");
+      if (adapter == null) throw new IllegalStateException("JsonAdapter isn't ready");
-      delegate.toJson(writer, value);
+      adapter.toJson(writer, value);
+    }
+
+    @Override public String toString() {
+      return adapter != null ? adapter.toString() : super.toString();
     }
   }
 }

moshi/src/test/java/com/squareup/moshi/DeferredAdapterTest.java

@@ -0,0 +1,117 @@
+/*
+ * Copyright (C) 2018 Square, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.squareup.moshi;
+
+import java.lang.annotation.Annotation;
+import java.lang.reflect.Type;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import javax.annotation.Nullable;
+import org.junit.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public final class DeferredAdapterTest {
+  /**
+   * When a type's JsonAdapter is circularly-dependent, Moshi creates a 'deferred adapter' to make
+   * the cycle work. It's important that any adapters that depend on this deferred adapter don't
+   * leak out until it's ready.
+   *
+   * <p>This test sets up a circular dependency BlueNode to GreenNode to BlueNode and then tries to
+   * use it before the BlueNode JSON adapter is built, but after the GreenNode adapter is built. It
+   * creates a similar cycle for BlueNode to RedNode to BlueNode so the order adapters are retrieved
+   * is insignificant.
+   */
+  @Test public void concurrentSafe() {
+    final List<Throwable> failures = new ArrayList<>();
+
+    JsonAdapter.Factory factory = new JsonAdapter.Factory() {
+      int redAndGreenCount = 0;
+
+      @Override public @Nullable JsonAdapter<?> create(
+          Type type, Set<? extends Annotation> annotations, final Moshi moshi) {
+        if ((type == RedNode.class || type == GreenNode.class) && redAndGreenCount++ == 1) {
+          doInAnotherThread(new Runnable() {
+            @Override public void run() {
+              GreenNode greenBlue = new GreenNode(new BlueNode(null, null));
+              assertThat(moshi.adapter(GreenNode.class).toJson(greenBlue))
+                  .isEqualTo("{\"blue\":{}}");
+
+              RedNode redBlue = new RedNode(new BlueNode(null, null));
+              assertThat(moshi.adapter(RedNode.class).toJson(redBlue))
+                  .isEqualTo("{\"blue\":{}}");
+            }
+          });
+        }
+        return null;
+      }
+    };
+
+    Moshi moshi = new Moshi.Builder()
+        .add(factory)
+        .build();
+
+    JsonAdapter<BlueNode> jsonAdapter = moshi.adapter(BlueNode.class);
+    assertThat(jsonAdapter.toJson(new BlueNode(new GreenNode(new BlueNode(null, null)), null)))
+        .isEqualTo("{\"green\":{\"blue\":{}}}");
+
+    assertThat(failures).isEmpty();
+  }
+
+  private void doInAnotherThread(Runnable runnable) {
+    ExecutorService executor = Executors.newSingleThreadExecutor();
+    Future<?> future = executor.submit(runnable);
+    executor.shutdown();
+    try {
+      future.get();
+    } catch (InterruptedException e) {
+      throw new RuntimeException(e);
+    } catch (ExecutionException e) {
+      throw new RuntimeException(e.getCause());
+    }
+  }
+
+  static class BlueNode {
+    @Nullable GreenNode green;
+    @Nullable RedNode red;
+
+    BlueNode(@Nullable GreenNode green, @Nullable RedNode red) {
+      this.green = green;
+      this.red = red;
+    }
+  }
+
+  static class RedNode {
+    @Nullable BlueNode blue;
+
+    RedNode(@Nullable BlueNode blue) {
+      this.blue = blue;
+    }
+  }
+
+  static class GreenNode {
+    @Nullable BlueNode blue;
+
+    GreenNode(@Nullable BlueNode blue) {
+      this.blue = blue;
+    }
+  }
+}