From 07a461a735316f15f9a78455e1c1c3caf91b2a3e Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Mon, 17 Oct 2016 23:58:54 +0200 Subject: [PATCH] BUG-4456: add RecursiveExtensionResolver 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 (cherry picked from commit bf55e6f6cc2a321af90aeacb974a1f7cf26be393) (cherry picked from commit 80c95e16025e2531b722edb3c486df75105f013f) --- .../stmt/reactor/BuildGlobalContext.java | 13 +- .../stmt/rfc6020/ExtensionStatementImpl.java | 28 ++--- .../stmt/rfc6020/RecursiveObjectLeaker.java | 115 ++++++++++++++++++ .../effective/EffectiveStatementBase.java | 4 + .../ExtensionEffectiveStatementImpl.java | 72 +++++++++-- .../UnknownEffectiveStatementBase.java | 11 +- .../yangtools/yang/stmt/test/Bug4456Test.java | 2 +- 7 files changed, 209 insertions(+), 36 deletions(-) create mode 100644 yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/RecursiveObjectLeaker.java diff --git a/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/BuildGlobalContext.java b/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/BuildGlobalContext.java index 90b9a6e653..dfed428295 100644 --- a/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/BuildGlobalContext.java +++ b/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/BuildGlobalContext.java @@ -42,6 +42,7 @@ import org.opendaylight.yangtools.yang.parser.spi.source.StatementStreamSource; 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.effective.EffectiveSchemaContext; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -183,10 +184,14 @@ class BuildGlobalContext extends NamespaceStorageSupport implements NamespaceBeh List> rootStatements = new ArrayList<>(sources.size()); List> rootEffectiveStatements = new ArrayList<>(sources.size()); - for (SourceSpecificContext source : sources) { - final RootStatementContext root = source.getRoot(); - rootStatements.add(root.buildDeclared()); - rootEffectiveStatements.add(root.buildEffective()); + try { + for (SourceSpecificContext source : sources) { + final RootStatementContext root = source.getRoot(); + rootStatements.add(root.buildDeclared()); + rootEffectiveStatements.add(root.buildEffective()); + } + } finally { + RecursiveObjectLeaker.cleanup(); } return new EffectiveSchemaContext(rootStatements, rootEffectiveStatements); diff --git a/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/ExtensionStatementImpl.java b/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/ExtensionStatementImpl.java index 74c6e67cb2..f9ec24290d 100644 --- a/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/ExtensionStatementImpl.java +++ b/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/ExtensionStatementImpl.java @@ -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; @@ -39,8 +37,6 @@ public class ExtensionStatementImpl extends AbstractDeclaredStatement imp } public static class Definition extends AbstractStatementSupport> { - private static final ThreadLocal>> BUILDING = new ThreadLocal<>(); - public Definition() { super(Rfc6020Mapping.EXTENSION); } @@ -57,24 +53,22 @@ public class ExtensionStatementImpl extends AbstractDeclaredStatement imp @Override public EffectiveStatement createEffective( - final StmtContext> ctx) { - Set> building = BUILDING.get(); - if (building == null) { - building = new HashSet<>(); - BUILDING.set(building); + final StmtContext> 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 index 0000000000..61d4f9f12d --- /dev/null +++ b/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/RecursiveObjectLeaker.java @@ -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>> STACK = new ThreadLocal<>(); + + private RecursiveObjectLeaker() { + throw new UnsupportedOperationException(); + } + + // Key is checked for identity + public static void beforeConstructor(final Object key) { + Deque> 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> stack = STACK.get(); + if (stack != null) { + final Entry 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> stack = STACK.get(); + Preconditions.checkState(stack != null, "No stack allocated when completing %s", key); + + final Entry 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 lookup(final Object key, final Class requiredClass) { + final Deque> stack = STACK.get(); + if (stack != null) { + for (Entry 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"); + } +} diff --git a/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/effective/EffectiveStatementBase.java b/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/effective/EffectiveStatementBase.java index 2af2b1fabe..2a621b2c9f 100644 --- a/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/effective/EffectiveStatementBase.java +++ b/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/effective/EffectiveStatementBase.java @@ -25,6 +25,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> implements EffectiveStatement { @@ -68,6 +69,9 @@ public abstract class EffectiveStatementBase> } substatementsInit.addAll(effectiveSubstatements); + // WARNING: this leaks an incompletely-initialized pbject + RecursiveObjectLeaker.inConstructor(this); + this.substatements = ImmutableList.copyOf(Collections2.transform(Collections2.filter(substatementsInit, IS_SUPPORTED_TO_BUILD_EFFECTIVE), StmtContextUtils.buildEffective())); } diff --git a/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/effective/ExtensionEffectiveStatementImpl.java b/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/effective/ExtensionEffectiveStatementImpl.java index 4a13515b8d..94e8e54683 100644 --- a/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/effective/ExtensionEffectiveStatementImpl.java +++ b/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/effective/ExtensionEffectiveStatementImpl.java @@ -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 implements ExtensionDefinition { + private static final class RecursionDetector extends ThreadLocal> { + boolean check(final ExtensionEffectiveStatementImpl current) { + final Deque stack = get(); + if (stack != null) { + for (ExtensionEffectiveStatementImpl s : stack) { + if (s == current) { + return true; + } + } + } + return false; + } + + void push(final ExtensionEffectiveStatementImpl current) { + Deque stack = get(); + if (stack == null) { + stack = new ArrayDeque<>(1); + set(stack); + } + + stack.push(current); + } + + void pop() { + Deque 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,14 +147,32 @@ public class ExtensionEffectiveStatementImpl extends AbstractEffectiveDocumented @Override public String toString() { - StringBuilder sb = new StringBuilder(ExtensionEffectiveStatementImpl.class.getSimpleName()); - sb.append("["); - sb.append("argument=").append(argument); - sb.append(", qname=").append(qname); - sb.append(", schemaPath=").append(schemaPath); - sb.append(", extensionSchemaNodes=").append(unknownNodes); - sb.append(", yin=").append(yin); - sb.append("]"); - return sb.toString(); + if (TOSTRING_DETECTOR.check(this)) { + return recursedToString(); + } + + TOSTRING_DETECTOR.push(this); + try { + StringBuilder sb = new StringBuilder(ExtensionEffectiveStatementImpl.class.getSimpleName()); + sb.append("["); + sb.append("argument=").append(argument); + sb.append(", qname=").append(qname); + sb.append(", schemaPath=").append(schemaPath); + sb.append(", extensionSchemaNodes=").append(unknownNodes); + sb.append(", yin=").append(yin); + sb.append("]"); + return sb.toString(); + } finally { + TOSTRING_DETECTOR.pop(); + } + } + + private String recursedToString() { + return ExtensionEffectiveStatementImpl.class.getSimpleName() + "[" + + "argument=" + argument + + ", qname=" + qname + + ", schemaPath=" + schemaPath + + ", yin=" + yin + + " ]"; } } diff --git a/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/effective/UnknownEffectiveStatementBase.java b/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/effective/UnknownEffectiveStatementBase.java index 738f70d555..9426efb0cc 100644 --- a/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/effective/UnknownEffectiveStatementBase.java +++ b/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/effective/UnknownEffectiveStatementBase.java @@ -43,7 +43,7 @@ public abstract class UnknownEffectiveStatementBase 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 extends AbstractEffective @Override public QName getNodeType() { - return nodeType; + return extension == null ? nodeType : extension.getQName(); } @Override @@ -99,13 +99,14 @@ public abstract class UnknownEffectiveStatementBase extends AbstractEffective @Override public String toString() { + final QName type = getNodeType(); + StringBuilder sb = new StringBuilder(); - sb.append(nodeType.getNamespace()); + sb.append(type.getNamespace()); sb.append(":"); - sb.append(nodeType.getLocalName()); + sb.append(type.getLocalName()); sb.append(" "); sb.append(nodeParameter); return sb.toString(); } - } diff --git a/yang/yang-parser-impl/src/test/java/org/opendaylight/yangtools/yang/stmt/test/Bug4456Test.java b/yang/yang-parser-impl/src/test/java/org/opendaylight/yangtools/yang/stmt/test/Bug4456Test.java index 5af276c546..88eb8cdba4 100644 --- a/yang/yang-parser-impl/src/test/java/org/opendaylight/yangtools/yang/stmt/test/Bug4456Test.java +++ b/yang/yang-parser-impl/src/test/java/org/opendaylight/yangtools/yang/stmt/test/Bug4456Test.java @@ -25,7 +25,7 @@ import org.opendaylight.yangtools.yang.parser.spi.meta.ReactorException; import org.opendaylight.yangtools.yang.parser.spi.source.SourceException; public class Bug4456Test { - @Test(expected=SourceException.class) + @Test public void test() throws IOException, URISyntaxException, SourceException, ReactorException { SchemaContext schema = StmtTestUtils.parseYangSources("/bugs/bug4456"); assertNotNull(schema); -- 2.36.6