BUG-4456: add RecursiveExtensionResolver 84/47084/1
authorRobert Varga <rovarga@cisco.com>
Mon, 17 Oct 2016 21:58:54 +0000 (23:58 +0200)
committerRobert Varga <rovarga@cisco.com>
Tue, 18 Oct 2016 15:43:04 +0000 (17:43 +0200)
This patch adds a hack which allows us to instantiate
recursive extensions. The solution is rather ugly, but
should be workable.

Change-Id: Ib5083804e5b49bd51c3b75c9905be39924e79ae5
Signed-off-by: Robert Varga <rovarga@cisco.com>
(cherry picked from commit bf55e6f6cc2a321af90aeacb974a1f7cf26be393)

yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/BuildGlobalContext.java
yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/ExtensionStatementImpl.java
yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/RecursiveObjectLeaker.java [new file with mode: 0644]
yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/effective/EffectiveStatementBase.java
yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/effective/ExtensionEffectiveStatementImpl.java
yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/effective/UnknownEffectiveStatementBase.java
yang/yang-parser-impl/src/test/java/org/opendaylight/yangtools/yang/stmt/Bug4456Test.java

index 8bc0936327441acc94cae0622ae5e33033d96e2c..62822474d7042f4659c9bde7afcaced862b000ef 100644 (file)
@@ -46,6 +46,7 @@ import org.opendaylight.yangtools.yang.parser.spi.source.SupportedFeaturesNamesp
 import org.opendaylight.yangtools.yang.parser.spi.validation.ValidationBundlesNamespace;
 import org.opendaylight.yangtools.yang.parser.spi.validation.ValidationBundlesNamespace.ValidationBundleType;
 import org.opendaylight.yangtools.yang.parser.stmt.reactor.SourceSpecificContext.PhaseCompletionProgress;
+import org.opendaylight.yangtools.yang.parser.stmt.rfc6020.RecursiveObjectLeaker;
 import org.opendaylight.yangtools.yang.parser.stmt.rfc6020.Utils;
 import org.opendaylight.yangtools.yang.parser.stmt.rfc6020.effective.EffectiveSchemaContext;
 import org.slf4j.Logger;
@@ -71,7 +72,7 @@ class BuildGlobalContext extends NamespaceStorageSupport implements NamespaceBeh
     private final boolean enabledSemanticVersions;
 
     public BuildGlobalContext(final Map<ModelProcessingPhase, StatementSupportBundle> supports,
-            StatementParserMode statementParserMode, final Predicate<QName> isFeatureSupported) {
+            final StatementParserMode statementParserMode, final Predicate<QName> isFeatureSupported) {
         super();
         this.supports = Preconditions.checkNotNull(supports, "BuildGlobalContext#supports cannot be null");
         Preconditions.checkNotNull(statementParserMode, "Statement parser mode must not be null.");
@@ -83,7 +84,7 @@ class BuildGlobalContext extends NamespaceStorageSupport implements NamespaceBeh
 
     public BuildGlobalContext(final Map<ModelProcessingPhase, StatementSupportBundle> supports,
             final Map<ValidationBundleType, Collection<?>> supportedValidation,
-            StatementParserMode statementParserMode, final Predicate<QName> isFeatureSupported) {
+            final StatementParserMode statementParserMode, final Predicate<QName> isFeatureSupported) {
         super();
         this.supports = Preconditions.checkNotNull(supports, "BuildGlobalContext#supports cannot be null");
         Preconditions.checkNotNull(statementParserMode, "Statement parser mode must not be null.");
@@ -216,6 +217,8 @@ class BuildGlobalContext extends NamespaceStorageSupport implements NamespaceBeh
             }
         } catch (SourceException ex) {
             throw new SomeModifiersUnresolvedException(currentPhase, sourceId, ex);
+        } finally {
+            RecursiveObjectLeaker.cleanup();
         }
 
         return new EffectiveSchemaContext(rootStatements, rootEffectiveStatements);
index 9a09f2b4abe1454ef16dc609524a5f57791cb2b3..ebcbc5b189b96697daa9ae2f3faf06732784810a 100644 (file)
@@ -7,8 +7,6 @@
  */
 package org.opendaylight.yangtools.yang.parser.stmt.rfc6020;
 
-import java.util.HashSet;
-import java.util.Set;
 import org.opendaylight.yangtools.yang.common.QName;
 import org.opendaylight.yangtools.yang.model.api.Rfc6020Mapping;
 import org.opendaylight.yangtools.yang.model.api.meta.EffectiveStatement;
@@ -22,7 +20,6 @@ import org.opendaylight.yangtools.yang.parser.spi.SubstatementValidator;
 import org.opendaylight.yangtools.yang.parser.spi.meta.AbstractDeclaredStatement;
 import org.opendaylight.yangtools.yang.parser.spi.meta.AbstractStatementSupport;
 import org.opendaylight.yangtools.yang.parser.spi.meta.StmtContext;
-import org.opendaylight.yangtools.yang.parser.spi.source.SourceException;
 import org.opendaylight.yangtools.yang.parser.stmt.rfc6020.effective.ExtensionEffectiveStatementImpl;
 
 public class ExtensionStatementImpl extends AbstractDeclaredStatement<QName> implements ExtensionStatement {
@@ -39,8 +36,6 @@ public class ExtensionStatementImpl extends AbstractDeclaredStatement<QName> imp
     }
 
     public static class Definition extends AbstractStatementSupport<QName,ExtensionStatement,EffectiveStatement<QName,ExtensionStatement>> {
-        private static final ThreadLocal<Set<StmtContext<?, ?, ?>>> BUILDING = new ThreadLocal<>();
-
         public Definition() {
             super(Rfc6020Mapping.EXTENSION);
         }
@@ -57,24 +52,22 @@ public class ExtensionStatementImpl extends AbstractDeclaredStatement<QName> imp
 
         @Override
         public EffectiveStatement<QName,ExtensionStatement> createEffective(
-                final StmtContext<QName,ExtensionStatement, EffectiveStatement<QName,ExtensionStatement>> ctx) {
-            Set<StmtContext<?, ?, ?>> building = BUILDING.get();
-            if (building == null) {
-                building = new HashSet<>();
-                BUILDING.set(building);
+                final StmtContext<QName,ExtensionStatement ,EffectiveStatement<QName,ExtensionStatement>> ctx) {
+
+            // Look at the thread-local leak in case we are invoked recursively
+            final ExtensionEffectiveStatementImpl existing = RecursiveObjectLeaker.lookup(ctx,
+                ExtensionEffectiveStatementImpl.class);
+            if (existing != null) {
+                // Careful! this not fully initialized!
+                return existing;
             }
 
-            SourceException.throwIf(building.contains(ctx), ctx.getStatementSourceReference(),
-                "Extension %s references itself", ctx.getStatementArgument());
-
-            building.add(ctx);
+            RecursiveObjectLeaker.beforeConstructor(ctx);
             try {
+                // This result is fine, we know it has been completely initialized
                 return new ExtensionEffectiveStatementImpl(ctx);
             } finally {
-                building.remove(ctx);
-                if (building.isEmpty()) {
-                    BUILDING.remove();
-                }
+                RecursiveObjectLeaker.afterConstructor(ctx);
             }
         }
 
diff --git a/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/RecursiveObjectLeaker.java b/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/RecursiveObjectLeaker.java
new file mode 100644 (file)
index 0000000..61d4f9f
--- /dev/null
@@ -0,0 +1,115 @@
+/*
+ * Copyright (c) 2015 Cisco Systems, Inc. and others.  All rights reserved.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v1.0 which accompanies this distribution,
+ * and is available at http://www.eclipse.org/legal/epl-v10.html
+ */
+package org.opendaylight.yangtools.yang.parser.stmt.rfc6020;
+
+import com.google.common.annotations.Beta;
+import com.google.common.base.Preconditions;
+import java.util.AbstractMap.SimpleEntry;
+import java.util.ArrayDeque;
+import java.util.Deque;
+import java.util.Map.Entry;
+import javax.annotation.Nullable;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Thread-local hack to make recursive extensions work without too much hassle. The idea is that prior to instantiating
+ * an extension, the definition object checks whether it is already present on the stack, recorded object is returned.
+ *
+ * If it is not, it will push itself to the stack as unresolved and invoke the constructor. The constructor's lowermost
+ * class calls to this class and if the topmost entry is not resolved, it will leak itself.
+ *
+ * Upon return from the constructor, the topmost entry is removed and if the queue is empty, the thread-local variable
+ * will be cleaned up.
+ *
+ * @author Robert Varga
+ */
+@Beta
+public final class RecursiveObjectLeaker {
+    // Logging note. Only keys passed can be logged, as objects beng resolved may not be properly constructed.
+    private static final Logger LOG = LoggerFactory.getLogger(RecursiveObjectLeaker.class);
+
+    // Initial value is set to null on purpose, so we do not allocate anything (aside the map)
+    private static final ThreadLocal<Deque<Entry<?, Object>>> STACK = new ThreadLocal<>();
+
+    private RecursiveObjectLeaker() {
+        throw new UnsupportedOperationException();
+    }
+
+    // Key is checked for identity
+    public static void beforeConstructor(final Object key) {
+        Deque<Entry<?, Object>> stack = STACK.get();
+        if (stack == null) {
+            // Biased: this class is expected to be rarely and shallowly used
+            stack = new ArrayDeque<>(1);
+            STACK.set(stack);
+        }
+
+        LOG.debug("Resolving key {}", key);
+        stack.push(new SimpleEntry<>(key, null));
+    }
+
+    // Can potentially store a 'null' mapping. Make sure cleanup() is called
+    public static void inConstructor(final Object obj) {
+        final Deque<Entry<?, Object>> stack = STACK.get();
+        if (stack != null) {
+            final Entry<?, Object> top = stack.peek();
+            if (top != null) {
+                if (top.getValue() == null) {
+                    LOG.debug("Resolved key {}", top.getKey());
+                    top.setValue(obj);
+                }
+            } else {
+                LOG.info("Cleaned stale empty stack", new Exception());
+                STACK.set(null);
+            }
+        } else {
+            LOG.trace("No thread stack");
+        }
+    }
+
+    // Make sure to call this from a finally block
+    public static void afterConstructor(final Object key) {
+        final Deque<Entry<?, Object>> stack = STACK.get();
+        Preconditions.checkState(stack != null, "No stack allocated when completing %s", key);
+
+        final Entry<?, Object> top = stack.pop();
+        if (stack.isEmpty()) {
+            LOG.trace("Removed empty thread stack");
+            STACK.set(null);
+        }
+
+        Preconditions.checkState(key == top.getKey(), "Expected key %s, have %s", top.getKey(), key);
+        Preconditions.checkState(top.getValue() != null, "");
+    }
+
+    // BEWARE: this method returns incpmpletely-initialized objects (that is the purpose of this class).
+    //
+    //         BE VERY CAREFUL WHAT OBJECT STATE YOU TOUCH
+    public static @Nullable <T> T lookup(final Object key, final Class<T> requiredClass) {
+        final Deque<Entry<?, Object>> stack = STACK.get();
+        if (stack != null) {
+            for (Entry<?, Object> e : stack) {
+                // Keys are treated as identities
+                if (key == e.getKey()) {
+                    Preconditions.checkState(e.getValue() != null, "Object for %s is not resolved", key);
+                    LOG.debug("Looked up key {}", e.getKey());
+                    return requiredClass.cast(e.getValue());
+                }
+            }
+        }
+
+        return null;
+    }
+
+    // Be sure to call this in from a finally block when bulk processing is done, so that this class can be unloaded
+    public static void cleanup() {
+        STACK.remove();
+        LOG.debug("Removed thread state");
+    }
+}
index 81374085f66da0ad9974985f5a67af9c506681c5..8580ef6d756a4ae179880982c9b7a5761a435586 100644 (file)
@@ -24,6 +24,7 @@ import org.opendaylight.yangtools.yang.model.api.meta.IdentifierNamespace;
 import org.opendaylight.yangtools.yang.parser.spi.meta.StmtContext;
 import org.opendaylight.yangtools.yang.parser.spi.meta.StmtContextUtils;
 import org.opendaylight.yangtools.yang.parser.stmt.reactor.StatementContextBase;
+import org.opendaylight.yangtools.yang.parser.stmt.rfc6020.RecursiveObjectLeaker;
 
 public abstract class EffectiveStatementBase<A, D extends DeclaredStatement<A>> implements EffectiveStatement<A, D> {
     private final List<? extends EffectiveStatement<?, ?>> substatements;
@@ -35,7 +36,6 @@ public abstract class EffectiveStatementBase<A, D extends DeclaredStatement<A>>
      *            context of statement.
      */
     protected EffectiveStatementBase(final StmtContext<A, D, ?> ctx) {
-
         final Collection<StatementContextBase<?, ?, ?>> effectiveSubstatements = ctx.effectiveSubstatements();
         final Collection<StatementContextBase<?, ?, ?>> substatementsInit = new ArrayList<>();
 
@@ -53,6 +53,9 @@ public abstract class EffectiveStatementBase<A, D extends DeclaredStatement<A>>
         }
         substatementsInit.addAll(effectiveSubstatements);
 
+        // WARNING: this leaks an incompletely-initialized pbject
+        RecursiveObjectLeaker.inConstructor(this);
+
         this.substatements = ImmutableList.copyOf(Collections2.transform(Collections2.filter(substatementsInit,
             StmtContext::isSupportedToBuildEffective), StatementContextBase::buildEffective));
     }
index fd9623b0bc3f0c59a7f06a387e2f9e7ae3922f42..d40c8ea519ad1342c64ab11ce600baf548a214fa 100644 (file)
@@ -8,7 +8,9 @@
 package org.opendaylight.yangtools.yang.parser.stmt.rfc6020.effective;
 
 import com.google.common.collect.ImmutableList;
+import java.util.ArrayDeque;
 import java.util.ArrayList;
+import java.util.Deque;
 import java.util.List;
 import java.util.Objects;
 import org.opendaylight.yangtools.yang.common.QName;
@@ -21,6 +23,40 @@ import org.opendaylight.yangtools.yang.parser.spi.meta.StmtContext;
 
 public class ExtensionEffectiveStatementImpl extends AbstractEffectiveDocumentedNode<QName, ExtensionStatement>
         implements ExtensionDefinition {
+    private static final class RecursionDetector extends ThreadLocal<Deque<ExtensionEffectiveStatementImpl>> {
+        boolean check(final ExtensionEffectiveStatementImpl current) {
+            final Deque<ExtensionEffectiveStatementImpl> stack = get();
+            if (stack != null) {
+                for (ExtensionEffectiveStatementImpl s : stack) {
+                    if (s == current) {
+                        return true;
+                    }
+                }
+            }
+            return false;
+        }
+
+        void push(final ExtensionEffectiveStatementImpl current) {
+            Deque<ExtensionEffectiveStatementImpl> stack = get();
+            if (stack == null) {
+                stack = new ArrayDeque<>(1);
+                set(stack);
+            }
+
+            stack.push(current);
+        }
+
+        void pop() {
+            Deque<ExtensionEffectiveStatementImpl> stack = get();
+            stack.pop();
+            if (stack.isEmpty()) {
+                remove();
+            }
+        }
+    }
+
+    private static final RecursionDetector TOSTRING_DETECTOR = new RecursionDetector();
+
     private final QName qname;
     private final String argument;
     private final SchemaPath schemaPath;
@@ -111,12 +147,30 @@ public class ExtensionEffectiveStatementImpl extends AbstractEffectiveDocumented
 
     @Override
     public String toString() {
+        if (TOSTRING_DETECTOR.check(this)) {
+            return recursedToString();
+        }
+
+        TOSTRING_DETECTOR.push(this);
+        try {
+            return ExtensionEffectiveStatementImpl.class.getSimpleName() + "[" +
+                    "argument=" + argument +
+                    ", qname=" + qname +
+                    ", schemaPath=" + schemaPath +
+                    ", extensionSchemaNodes=" + unknownNodes +
+                    ", yin=" + yin +
+                    "]";
+        } finally {
+            TOSTRING_DETECTOR.pop();
+        }
+    }
+
+    private String recursedToString() {
         return ExtensionEffectiveStatementImpl.class.getSimpleName() + "[" +
                 "argument=" + argument +
                 ", qname=" + qname +
                 ", schemaPath=" + schemaPath +
-                ", extensionSchemaNodes=" + unknownNodes +
                 ", yin=" + yin +
-                "]";
+                " <RECURSIVE> ]";
     }
 }
index 709d0eb94764f80c06e86022c0353d3a7e4c820d..1fbf48fc3dc02148462dd5940538aa82fe783721 100644 (file)
@@ -43,7 +43,7 @@ public abstract class UnknownEffectiveStatementBase<A> extends AbstractEffective
             nodeType = ctx.getPublicDefinition().getArgumentName();
         } else {
             extension = (ExtensionEffectiveStatementImpl) extensionInit.buildEffective();
-            nodeType = extension.getQName();
+            nodeType = null;
         }
 
         // initCopyType
@@ -74,7 +74,7 @@ public abstract class UnknownEffectiveStatementBase<A> extends AbstractEffective
 
     @Override
     public QName getNodeType() {
-        return nodeType;
+        return extension == null ? nodeType : extension.getQName();
     }
 
     @Override
@@ -99,11 +99,9 @@ public abstract class UnknownEffectiveStatementBase<A> extends AbstractEffective
 
     @Override
     public String toString() {
-        return String.valueOf(nodeType.getNamespace()) +
-                ":" +
-                nodeType.getLocalName() +
-                " " +
-                nodeParameter;
-    }
+        final QName type = getNodeType();
 
+        return String.valueOf(type.getNamespace()) +
+                ":" + type.getLocalName() + " " + nodeParameter;
+    }
 }
index 3fa3f208cfca877944339f2ceec2d8f6e93e7565..01cea68b48ceec021899fb95e044f3a92aca4135 100644 (file)
@@ -22,11 +22,10 @@ import org.opendaylight.yangtools.yang.model.api.Module;
 import org.opendaylight.yangtools.yang.model.api.SchemaContext;
 import org.opendaylight.yangtools.yang.model.api.UnknownSchemaNode;
 import org.opendaylight.yangtools.yang.parser.spi.meta.ReactorException;
-import org.opendaylight.yangtools.yang.parser.spi.meta.SomeModifiersUnresolvedException;
 import org.opendaylight.yangtools.yang.parser.spi.source.SourceException;
 
 public class Bug4456Test {
-    @Test(expected=SomeModifiersUnresolvedException.class)
+    @Test
     public void test() throws IOException, URISyntaxException, SourceException, ReactorException {
         SchemaContext schema = StmtTestUtils.parseYangSources("/bugs/bug4456");
         assertNotNull(schema);