diff --git a/moshi/src/main/java/com/squareup/moshi/Moshi.java b/moshi/src/main/java/com/squareup/moshi/Moshi.java index 26db316..6b98f61 100644 --- a/moshi/src/main/java/com/squareup/moshi/Moshi.java +++ b/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 factories; - private final ThreadLocal>> reentrantCalls = new ThreadLocal<>(); + private final ThreadLocal lookupChainThreadLocal = new ThreadLocal<>(); private final Map> adapterCache = new LinkedHashMap<>(); Moshi(Builder builder) { @@ -118,52 +121,35 @@ public final class Moshi { if (result != null) return (JsonAdapter) result; } - // Short-circuit if this is a reentrant call. - List> deferredAdapters = reentrantCalls.get(); - if (deferredAdapters != null) { - for (int i = 0, size = deferredAdapters.size(); i < size; i++) { - DeferredAdapter deferredAdapter = deferredAdapters.get(i); - if (deferredAdapter.cacheKey.equals(cacheKey)) { - return (JsonAdapter) deferredAdapter; - } - } - } else { - deferredAdapters = new ArrayList<>(); - reentrantCalls.set(deferredAdapters); + LookupChain lookupChain = lookupChainThreadLocal.get(); + if (lookupChain == null) { + lookupChain = new LookupChain(); + lookupChainThreadLocal.set(lookupChain); } - // Prepare for re-entrant calls, then ask each factory to create a type adapter. - DeferredAdapter deferredAdapter = new DeferredAdapter<>(type, fieldName, cacheKey); - - deferredAdapters.add(deferredAdapter); - int lastIndex = deferredAdapters.size() - 1; + boolean success = false; + JsonAdapter adapterFromCall = lookupChain.push(type, fieldName, cacheKey); 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 result = (JsonAdapter) factories.get(i).create(type, annotations, this); - if (result != null) { - 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(); - } - } + if (result == null) continue; - throw new IllegalArgumentException( - "No JsonAdapter for " + typeAnnotatedWithAnnotations(type, annotations)); + // Success! Notify the LookupChain so it is cached and can be used by re-entrant calls. + 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> 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 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. - * 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. + * A possibly-reentrant chain of lookups a JSON adapter. * - *

Typically this is necessary in self-referential object models, such as an {@code Employee} - * class that has a {@code List} field for an organization's management hierarchy. + *

We keep track of the current stack of lookups: we may start by looking up the JSON adapter + * 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. + * + *

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} field for an organization's management hierarchy. + * + *

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 extends JsonAdapter { + final class LookupChain { + final List> callLookups = new ArrayList<>(); + final Deque> 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. + */ + JsonAdapter 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 hit = (Lookup) 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 lookup = new Lookup<>(type, fieldName, cacheKey); + callLookups.add(lookup); + stack.add(lookup); + return null; + } + + /** Sets the adapter result of the current lookup. */ + void adapterFound(JsonAdapter result) { + Lookup currentLookup = (Lookup) 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) lookup).adapter = (JsonAdapter) 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> 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 extends JsonAdapter { final Type type; final @Nullable String fieldName; - @Nullable Object cacheKey; - private @Nullable JsonAdapter delegate; + final Object cacheKey; + @Nullable JsonAdapter 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 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"); - return delegate.fromJson(reader); + if (adapter == null) throw new IllegalStateException("JsonAdapter isn't ready"); + 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"); - delegate.toJson(writer, value); + if (adapter == null) throw new IllegalStateException("JsonAdapter isn't ready"); + adapter.toJson(writer, value); + } + + @Override public String toString() { + return adapter != null ? adapter.toString() : super.toString(); } } } diff --git a/moshi/src/test/java/com/squareup/moshi/DeferredAdapterTest.java b/moshi/src/test/java/com/squareup/moshi/DeferredAdapterTest.java new file mode 100644 index 0000000..daeea13 --- /dev/null +++ b/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. + * + *

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 failures = new ArrayList<>(); + + JsonAdapter.Factory factory = new JsonAdapter.Factory() { + int redAndGreenCount = 0; + + @Override public @Nullable JsonAdapter create( + Type type, Set 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 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; + } + } +}