refactor: replace ThreadLocal with method argument

Set tracks types we've visited as we recurse.
Netflix · Jan 4, 2017 · a2b8f2f · a2b8f2f
1 parent eed5c93
commit a2b8f2f
Show file tree

Hide file tree

Showing 10 changed files with 121 additions and 41 deletions.
diff --git a/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowListTypeMapper.java b/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowListTypeMapper.java
@@ -25,7 +25,10 @@
 import com.netflix.hollow.core.write.HollowTypeWriteState;
 import com.netflix.hollow.core.write.HollowWriteRecord;
 import java.lang.reflect.ParameterizedType;
+import java.lang.reflect.Type;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 
 public class HollowListTypeMapper extends HollowTypeMapper {
 
@@ -37,8 +40,8 @@ public class HollowListTypeMapper extends HollowTypeMapper {
 
     private final HollowTypeMapper elementMapper;
 
-    public HollowListTypeMapper(HollowObjectMapper parentMapper, ParameterizedType type, String declaredName, boolean ignoreListOrdering) {
-        this.elementMapper = parentMapper.getTypeMapper(type.getActualTypeArguments()[0], null, null);
+    public HollowListTypeMapper(HollowObjectMapper parentMapper, ParameterizedType type, String declaredName, boolean ignoreListOrdering, Set<Type> visited) {
+        this.elementMapper = parentMapper.getTypeMapper(type.getActualTypeArguments()[0], null, null, visited);
         String typeName = declaredName != null ? declaredName : getDefaultTypeName(type);
         this.schema = new HollowListSchema(typeName, elementMapper.getTypeName());
         this.ignoreListOrdering = ignoreListOrdering;

diff --git a/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowMapTypeMapper.java b/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowMapTypeMapper.java
@@ -26,7 +26,9 @@
 import com.netflix.hollow.core.write.HollowWriteRecord;
 import com.netflix.hollow.core.write.HollowWriteStateEngine;
 import java.lang.reflect.ParameterizedType;
+import java.lang.reflect.Type;
 import java.util.Map;
+import java.util.Set;
 
 public class HollowMapTypeMapper extends HollowTypeMapper {
 
@@ -38,9 +40,9 @@ public class HollowMapTypeMapper extends HollowTypeMapper {
     private HollowTypeMapper keyMapper;
     private HollowTypeMapper valueMapper;
 
-    public HollowMapTypeMapper(HollowObjectMapper parentMapper, ParameterizedType type, String declaredName, String[] hashKeyFieldPaths, HollowWriteStateEngine stateEngine, boolean useDefaultHashKeys) {
-        this.keyMapper = parentMapper.getTypeMapper(type.getActualTypeArguments()[0], null, null);
-        this.valueMapper = parentMapper.getTypeMapper(type.getActualTypeArguments()[1], null, null);
+    public HollowMapTypeMapper(HollowObjectMapper parentMapper, ParameterizedType type, String declaredName, String[] hashKeyFieldPaths, HollowWriteStateEngine stateEngine, boolean useDefaultHashKeys, Set<Type> visited) {
+        this.keyMapper = parentMapper.getTypeMapper(type.getActualTypeArguments()[0], null, null, visited);
+        this.valueMapper = parentMapper.getTypeMapper(type.getActualTypeArguments()[1], null, null, visited);
         String typeName = declaredName != null ? declaredName : getDefaultTypeName(type);
 
         if(hashKeyFieldPaths == null && useDefaultHashKeys && (keyMapper instanceof HollowObjectTypeMapper))

diff --git a/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectMapper.java b/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectMapper.java
@@ -66,6 +66,9 @@ public void initializeTypeState(Class<?> clazz) {
     }
 
     HollowTypeMapper getTypeMapper(Type type, String declaredName, String[] hashKeyFieldPaths) {
+        return getTypeMapper(type, declaredName, hashKeyFieldPaths, new HashSet<Type>());
+    }
+    HollowTypeMapper getTypeMapper(Type type, String declaredName, String[] hashKeyFieldPaths, Set<Type> visited) {
         String typeName = declaredName != null ? declaredName : HollowObjectTypeMapper.getDefaultTypeName(type);
 
         HollowTypeMapper typeMapper = typeMappers.get(typeName);
@@ -77,17 +80,17 @@ HollowTypeMapper getTypeMapper(Type type, String declaredName, String[] hashKeyF
                 Class<?> clazz = (Class<?>) parameterizedType.getRawType();
 
                 if(List.class.isAssignableFrom(clazz)) {
-                    typeMapper = new HollowListTypeMapper(this, parameterizedType, declaredName, ignoreListOrdering);
+                    typeMapper = new HollowListTypeMapper(this, parameterizedType, declaredName, ignoreListOrdering, visited);
                 } else if(Set.class.isAssignableFrom(clazz)) {
-                    typeMapper = new HollowSetTypeMapper(this, parameterizedType, declaredName, hashKeyFieldPaths, stateEngine, useDefaultHashKeys);
+                    typeMapper = new HollowSetTypeMapper(this, parameterizedType, declaredName, hashKeyFieldPaths, stateEngine, useDefaultHashKeys, visited);
                 } else if(Map.class.isAssignableFrom(clazz)) {
-                    typeMapper = new HollowMapTypeMapper(this, parameterizedType, declaredName, hashKeyFieldPaths, stateEngine, useDefaultHashKeys);
+                    typeMapper = new HollowMapTypeMapper(this, parameterizedType, declaredName, hashKeyFieldPaths, stateEngine, useDefaultHashKeys, visited);
                 } else {
-                    return getTypeMapper(clazz, declaredName, hashKeyFieldPaths);
+                    return getTypeMapper(clazz, declaredName, hashKeyFieldPaths, visited);
                 }
 
             } else {
-                typeMapper = new HollowObjectTypeMapper(this, (Class<?>)type, declaredName);
+                typeMapper = new HollowObjectTypeMapper(this, (Class<?>)type, declaredName, visited);
             }
 
             HollowTypeMapper existing = typeMappers.putIfAbsent(typeName, typeMapper);

diff --git a/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectTypeMapper.java b/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectTypeMapper.java
@@ -39,12 +39,6 @@
 public class HollowObjectTypeMapper extends HollowTypeMapper {
 
     private static final Unsafe unsafe = HollowUnsafeHandle.getUnsafe();
-    private static final ThreadLocal<Set<Class<?>>> visitedClasses = new ThreadLocal<Set<Class<?>>>() {
-        @Override
-        protected Set<Class<?>> initialValue() {
-            return new HashSet<>();
-        }
-    };
     private final HollowObjectMapper parentMapper;
 
     private final String typeName;
@@ -56,7 +50,7 @@ protected Set<Class<?>> initialValue() {
 
     private final List<MappedField> mappedFields;
 
-    public HollowObjectTypeMapper(HollowObjectMapper parentMapper, Class<?> clazz, String declaredTypeName) {
+    public HollowObjectTypeMapper(HollowObjectMapper parentMapper, Class<?> clazz, String declaredTypeName, Set<Type> visited) {
         this.parentMapper = parentMapper;
         this.clazz = clazz;
         this.typeName = declaredTypeName != null ? declaredTypeName : getDefaultTypeName(clazz);
@@ -73,7 +67,7 @@ public HollowObjectTypeMapper(HollowObjectMapper parentMapper, Class<?> clazz, S
 
             for(int i=0;i<declaredFields.length;i++) {
                 if(!Modifier.isStatic(declaredFields[i].getModifiers()) && !"__assigned_ordinal".equals(declaredFields[i].getName())) {
-                    mappedFields.add(new MappedField(declaredFields[i]));
+                    mappedFields.add(new MappedField(declaredFields[i], visited));
                 }
             }
 
@@ -170,6 +164,9 @@ private class MappedField {
         private final boolean isEnumNameField;
 
         private MappedField(Field f) {
+            this(f, new HashSet<Type>());
+        }
+        private MappedField(Field f, Set<Type> visitedTypes) {
             this.fieldOffset = unsafe.objectFieldOffset(f);
             this.fieldName = f.getName();
             this.type = f.getGenericType();
@@ -196,14 +193,14 @@ private MappedField(Field f) {
                 fieldType = FieldType.STRING;
             } else {
                 fieldType = FieldType.REFERENCE;
-                if(!visitedClasses.get().contains(f.getType())) {
-                    // guard recursion: track visited classes in a threadlocal so we can detect circular references as we recurse
-                    visitedClasses.get().add(f.getType());
-                    subTypeMapper = parentMapper.getTypeMapper(type, typeNameAnnotation != null ? typeNameAnnotation.name() : null, hashKeyAnnotation != null ? hashKeyAnnotation.fields() : null);
-                    visitedClasses.get().remove(f.getType());
-                } else {
+                if(visitedTypes.contains(this.type)){
                     throw new IllegalStateException("circular reference detected on field " + f + "; this type of relationship is not supported");
                 }
+                // guard recursion here
+                visitedTypes.add(this.type);
+                subTypeMapper = parentMapper.getTypeMapper(type, typeNameAnnotation != null ? typeNameAnnotation.name() : null, hashKeyAnnotation != null ? hashKeyAnnotation.fields() : null, visitedTypes);
+                // once we've safely returned from a leaf node in recursion, we can remove this MappedField's type
+                visitedTypes.remove(this.type);
             }
 
             this.subTypeMapper = subTypeMapper;

diff --git a/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowSetTypeMapper.java b/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowSetTypeMapper.java
@@ -26,6 +26,7 @@
 import com.netflix.hollow.core.write.HollowWriteRecord;
 import com.netflix.hollow.core.write.HollowWriteStateEngine;
 import java.lang.reflect.ParameterizedType;
+import java.lang.reflect.Type;
 import java.util.Set;
 
 public class HollowSetTypeMapper extends HollowTypeMapper {
@@ -37,8 +38,8 @@ public class HollowSetTypeMapper extends HollowTypeMapper {
 
     private final HollowTypeMapper elementMapper;
 
-    public HollowSetTypeMapper(HollowObjectMapper parentMapper, ParameterizedType type, String declaredName, String[] hashKeyFieldPaths, HollowWriteStateEngine stateEngine, boolean useDefaultHashKeys) {
-        this.elementMapper = parentMapper.getTypeMapper(type.getActualTypeArguments()[0], null, null);
+    public HollowSetTypeMapper(HollowObjectMapper parentMapper, ParameterizedType type, String declaredName, String[] hashKeyFieldPaths, HollowWriteStateEngine stateEngine, boolean useDefaultHashKeys, Set<Type> visited) {
+        this.elementMapper = parentMapper.getTypeMapper(type.getActualTypeArguments()[0], null, null, visited);
         String typeName = declaredName != null ? declaredName : getDefaultTypeName(type);
 
         if(hashKeyFieldPaths == null && useDefaultHashKeys && (elementMapper instanceof HollowObjectTypeMapper))

diff --git a/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/DirectCircularReference.java b/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/DirectCircularReference.java
@@ -1,21 +1,16 @@
 package com.netflix.hollow.core.write.objectmapper;
 
-import java.util.Collections;
-import java.util.List;
 
 /**
  * Sample type that represents a direct circular reference between 2 classes.
  */
 public class DirectCircularReference {
 
     private final String name;
-    private final List<DirectCircularReference> children;
+    private final DirectCircularReference child;
 
-    public DirectCircularReference(String name) {
-        this(name, Collections.<DirectCircularReference>emptyList());
-    }
-    public DirectCircularReference(String name, List<DirectCircularReference> children) {
+    public DirectCircularReference(String name, DirectCircularReference child) {
         this.name = name;
-        this.children = children;
+        this.child = child;
     }
 }
diff --git a/...src/test/java/com/netflix/hollow/core/write/objectmapper/DirectListCircularReference.java b/...src/test/java/com/netflix/hollow/core/write/objectmapper/DirectListCircularReference.java
@@ -0,0 +1,18 @@
+package com.netflix.hollow.core.write.objectmapper;
+
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * Sample type that represents a direct circular reference between 2 classes, with a List containing the child.
+ */
+public class DirectListCircularReference {
+
+    private final String name;
+    private final List<DirectListCircularReference> children;
+
+    public DirectListCircularReference(String name, List<DirectListCircularReference> children) {
+        this.name = name;
+        this.children = children;
+    }
+}
diff --git a/.../src/test/java/com/netflix/hollow/core/write/objectmapper/DirectMapCircularReference.java b/.../src/test/java/com/netflix/hollow/core/write/objectmapper/DirectMapCircularReference.java
@@ -0,0 +1,17 @@
+package com.netflix.hollow.core.write.objectmapper;
+
+import java.util.Map;
+
+/**
+ * Sample type that represents a direct circular reference between 2 classes, with a Map containing the child.
+ */
+public class DirectMapCircularReference {
+
+    private final String name;
+    private final Map<String, DirectMapCircularReference> children;
+
+    public DirectMapCircularReference(String name, Map<String, DirectMapCircularReference> children) {
+        this.name = name;
+        this.children = children;
+    }
+}
diff --git a/.../src/test/java/com/netflix/hollow/core/write/objectmapper/DirectSetCircularReference.java b/.../src/test/java/com/netflix/hollow/core/write/objectmapper/DirectSetCircularReference.java
@@ -0,0 +1,17 @@
+package com.netflix.hollow.core.write.objectmapper;
+
+import java.util.Set;
+
+/**
+ * Sample type that represents a direct circular reference between 2 classes, with a Set containing the child.
+ */
+public class DirectSetCircularReference {
+
+    private final String name;
+    private final Set<DirectSetCircularReference> children;
+
+    public DirectSetCircularReference(String name, Set<DirectSetCircularReference> children) {
+        this.name = name;
+        this.children = children;
+    }
+}
diff --git a/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/HollowObjectMapperTest.java b/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/HollowObjectMapperTest.java
@@ -76,17 +76,44 @@ public void testEnumAndInlineClass() throws IOException {
         Assert.assertEquals(3, subObj.getInt("val2"));
     }
 
-    @Test(expected = IllegalStateException.class)
+    @Test
     public void testMappingCircularReference() throws IOException {
-        HollowObjectMapper mapper = new HollowObjectMapper(writeStateEngine);
-        mapper.initializeTypeState(DirectCircularReference.class);
+        assertExpectedFailureMappingType(DirectCircularReference.class, "child");
     }
-    @Test(expected = IllegalStateException.class)
-    public void testMappingDeeperCircularReference() throws IOException {
-        HollowObjectMapper mapper = new HollowObjectMapper(writeStateEngine);
-        mapper.initializeTypeState(IndirectCircularReference.TypeE.class);
+    @Test
+    public void testMappingCircularReferenceList() throws IOException {
+        assertExpectedFailureMappingType(DirectListCircularReference.class, "children");
+    }
+    @Test
+    public void testMappingCircularReferenceSet() throws IOException {
+        assertExpectedFailureMappingType(DirectSetCircularReference.class, "children");
+    }
+    @Test
+    public void testMappingCircularReferenceMap() throws IOException {
+        assertExpectedFailureMappingType(DirectMapCircularReference.class, "children");
+    }
+    @Test
+    public void testMappingIndirectircularReference() throws IOException {
+        assertExpectedFailureMappingType(IndirectCircularReference.TypeE.class, "f");
     }
 
+    /**
+     * Convenience method for experimenting with {@link HollowObjectMapper#initializeTypeState(Class)}
+     * on classes we know should fail due to circular references, confirming the exception message is correct.
+     *
+     * @param clazz class to initialize
+     * @param fieldName the name of the field that should trip the circular reference detection
+     */
+    protected void assertExpectedFailureMappingType(Class<?> clazz, String fieldName) {
+        final String expected = clazz.getSimpleName() + "." + fieldName;
+        try {
+            HollowObjectMapper mapper = new HollowObjectMapper(writeStateEngine);
+            mapper.initializeTypeState(clazz);
+        } catch (IllegalStateException e) {
+
+            Assert.assertTrue(String.format("missing expected fieldname %s in the message, was %s", expected, e.getMessage()), e.getMessage().contains(expected));
+        }
+    }
     private Map<String, List<Integer>> map(Object... keyValues) {
         Map<String, List<Integer>> map = new HashMap<String, List<Integer>>();
         int i = 0;