From bf55e6f6cc2a321af90aeacb974a1f7cf26be393 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 --- .../stmt/reactor/BuildGlobalContext.java | 7 +- .../stmt/rfc6020/ExtensionStatementImpl.java | 29 ++--- .../stmt/rfc6020/RecursiveObjectLeaker.java | 115 ++++++++++++++++++ .../effective/EffectiveStatementBase.java | 5 +- .../ExtensionEffectiveStatementImpl.java | 58 ++++++++- .../UnknownEffectiveStatementBase.java | 14 +-- .../yangtools/yang/stmt/Bug4456Test.java | 3 +- 7 files changed, 198 insertions(+), 33 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 8bc0936327..62822474d7 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 @@ -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 supports, - StatementParserMode statementParserMode, final Predicate isFeatureSupported) { + final StatementParserMode statementParserMode, final Predicate 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 supports, final Map> supportedValidation, - StatementParserMode statementParserMode, final Predicate isFeatureSupported) { + final StatementParserMode statementParserMode, final Predicate 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); 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 9a09f2b4ab..ebcbc5b189 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; @@ -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 implements ExtensionStatement { @@ -39,8 +36,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 +52,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 81374085f6..8580ef6d75 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 @@ -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> implements EffectiveStatement { private final List> substatements; @@ -35,7 +36,6 @@ public abstract class EffectiveStatementBase> * context of statement. */ protected EffectiveStatementBase(final StmtContext ctx) { - final Collection> effectiveSubstatements = ctx.effectiveSubstatements(); final Collection> substatementsInit = new ArrayList<>(); @@ -53,6 +53,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, StmtContext::isSupportedToBuildEffective), StatementContextBase::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 fd9623b0bc..d40c8ea519 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,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 + - "]"; + " ]"; } } 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 709d0eb947..1fbf48fc3d 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,11 +99,9 @@ public abstract class UnknownEffectiveStatementBase 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; + } } diff --git a/yang/yang-parser-impl/src/test/java/org/opendaylight/yangtools/yang/stmt/Bug4456Test.java b/yang/yang-parser-impl/src/test/java/org/opendaylight/yangtools/yang/stmt/Bug4456Test.java index 3fa3f208cf..01cea68b48 100644 --- a/yang/yang-parser-impl/src/test/java/org/opendaylight/yangtools/yang/stmt/Bug4456Test.java +++ b/yang/yang-parser-impl/src/test/java/org/opendaylight/yangtools/yang/stmt/Bug4456Test.java @@ -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); -- 2.36.6