From 72a8d46419e1884d9969ba534992c88c98510a9d Mon Sep 17 00:00:00 2001 From: Stian Soiland-Reyes Date: Sat, 21 Mar 2015 02:14:49 +0000 Subject: [PATCH 1/4] JENA-904 test LPDRuleEngine leak of activeInterpreters --- .../rulesys/impl/TestLPBRuleEngineLeak.java | 171 ++++++++++++++++++ .../reasoner/rulesys/test/TestPackage.java | 4 + 2 files changed, 175 insertions(+) create mode 100644 jena-core/src/test/java/com/hp/hpl/jena/reasoner/rulesys/impl/TestLPBRuleEngineLeak.java diff --git a/jena-core/src/test/java/com/hp/hpl/jena/reasoner/rulesys/impl/TestLPBRuleEngineLeak.java b/jena-core/src/test/java/com/hp/hpl/jena/reasoner/rulesys/impl/TestLPBRuleEngineLeak.java new file mode 100644 index 00000000000..5230249aabd --- /dev/null +++ b/jena-core/src/test/java/com/hp/hpl/jena/reasoner/rulesys/impl/TestLPBRuleEngineLeak.java @@ -0,0 +1,171 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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.hp.hpl.jena.reasoner.rulesys.impl; + +import java.lang.reflect.Field; +import java.util.List; + +import junit.framework.TestCase; +import junit.framework.TestSuite; + +import org.junit.Test; + +import com.hp.hpl.jena.graph.Factory; +import com.hp.hpl.jena.graph.Graph; +import com.hp.hpl.jena.graph.Node; +import com.hp.hpl.jena.graph.NodeFactory; +import com.hp.hpl.jena.graph.Triple; +import com.hp.hpl.jena.reasoner.rulesys.FBRuleInfGraph; +import com.hp.hpl.jena.reasoner.rulesys.FBRuleReasoner; +import com.hp.hpl.jena.reasoner.rulesys.Rule; +import com.hp.hpl.jena.util.iterator.ExtendedIterator; +import com.hp.hpl.jena.vocabulary.RDF; +import com.hp.hpl.jena.vocabulary.RDFS; + +public class TestLPBRuleEngineLeak extends TestCase { + public static TestSuite suite() { + return new TestSuite(TestLPBRuleEngineLeak.class, "TestLPBRuleEngineLeak"); + } + + protected Node a = NodeFactory.createURI("a"); + protected Node b = NodeFactory.createURI("b"); + protected Node nohit = NodeFactory.createURI("nohit"); + protected Node p = NodeFactory.createURI("p"); + protected Node C1 = NodeFactory.createURI("C1"); + protected Node C2 = NodeFactory.createURI("C2"); + protected Node ty = RDF.Nodes.type; + + public FBRuleReasoner createReasoner(List rules) { + FBRuleReasoner reasoner = new FBRuleReasoner(rules); + reasoner.tablePredicate(RDFS.Nodes.subClassOf); + reasoner.tablePredicate(RDF.Nodes.type); + reasoner.tablePredicate(p); + return reasoner; + } + + @Test + public void testNotLeakingActiveInterpreters() throws Exception { + Graph data = Factory.createGraphMem(); + data.add(new Triple(a, ty, C1)); + data.add(new Triple(b, ty, C1)); + List rules = Rule + .parseRules("[r1: (?x p ?t) <- (?x rdf:type C1), makeInstance(?x, p, C2, ?t)]" + + "[r2: (?t rdf:type C2) <- (?x rdf:type C1), makeInstance(?x, p, C2, ?t)]"); + + FBRuleInfGraph infgraph = (FBRuleInfGraph) createReasoner(rules).bind( + data); + + LPBRuleEngine engine = getEngineForGraph(infgraph); + assertEquals(0, engine.activeInterpreters.size()); + assertEquals(0, engine.tabledGoals.size()); + + // we ask for a non-hit -- it works, but only because we call it.hasNext() + ExtendedIterator it = infgraph.find(nohit, ty, C1); + assertFalse(it.hasNext()); + it.close(); + assertEquals(0, engine.activeInterpreters.size()); + + // and again. + // Ensure this is not cached by asking for a different triple pattern + ExtendedIterator it2 = infgraph.find(nohit, ty, C2); + // uuups, forgot to call it.hasNext(). But .close() should tidy + it2.close(); + assertEquals(0, engine.activeInterpreters.size()); + + + // OK, let's ask for something that is in the graph + + ExtendedIterator it3 = infgraph.find(a, ty, C1); + assertTrue(it3.hasNext()); + assertEquals(a, it3.next().getMatchSubject()); + + // .. and what if we forget to call next() to consume b? + // (e.g. return from a method with the first hit) + + // this should be enough + it3.close(); + // without leaks of activeInterpreters + assertEquals(0, engine.activeInterpreters.size()); + } + + @Test + public void testTabledGoalsCacheHits() throws Exception { + Graph data = Factory.createGraphMem(); + data.add(new Triple(a, ty, C1)); + List rules = Rule + .parseRules("[r1: (?x p ?t) <- (?x rdf:type C1), makeInstance(?x, p, C2, ?t)]" + + "[r2: (?t rdf:type C2) <- (?x rdf:type C1), makeInstance(?x, p, C2, ?t)]"); + + FBRuleInfGraph infgraph = (FBRuleInfGraph) createReasoner(rules).bind( + data); + + LPBRuleEngine engine = getEngineForGraph(infgraph); + assertEquals(0, engine.activeInterpreters.size()); + assertEquals(0, engine.tabledGoals.size()); + + ExtendedIterator it = infgraph.find(a, ty, C1); + while (it.hasNext()) { + it.next(); + // FIXME: Why do I need to consume all from the iterator + // to avoid leaking activeInterpreters? Calling .close() + // below should have been enough. + } + it.close(); + // how many were cached + assertEquals(1, engine.tabledGoals.size()); + // and no leaks of activeInterpreters + assertEquals(0, engine.activeInterpreters.size()); + + // Now ask again: + it = infgraph.find(a, ty, C1); + while (it.hasNext()) { + it.next(); + } + it.close(); + // if it was a cache hit, no change here: + assertEquals(1, engine.tabledGoals.size()); + assertEquals(0, engine.activeInterpreters.size()); + } + + /** + * Use introspection to get to the LPBRuleEngine. + *

+ * We are crossing package boundaries and therefore this test would always + * be in the wrong package for either FBRuleInfGraph or LPBRuleEngine. + *

+ * This method should only be used for test purposes. + * + * @param infgraph + * @return + * @throws SecurityException + * @throws NoSuchFieldException + * @throws IllegalArgumentException + * @throws IllegalAccessException + */ + private LPBRuleEngine getEngineForGraph(FBRuleInfGraph infgraph) + throws NoSuchFieldException, SecurityException, + IllegalArgumentException, IllegalAccessException { + Field bEngine = FBRuleInfGraph.class.getDeclaredField("bEngine"); + bEngine.setAccessible(true); + LPBRuleEngine engine = (LPBRuleEngine) bEngine.get(infgraph); + return engine; + } + +} diff --git a/jena-core/src/test/java/com/hp/hpl/jena/reasoner/rulesys/test/TestPackage.java b/jena-core/src/test/java/com/hp/hpl/jena/reasoner/rulesys/test/TestPackage.java index 5334dc251d7..e8e1587baf2 100755 --- a/jena-core/src/test/java/com/hp/hpl/jena/reasoner/rulesys/test/TestPackage.java +++ b/jena-core/src/test/java/com/hp/hpl/jena/reasoner/rulesys/test/TestPackage.java @@ -20,9 +20,12 @@ import junit.framework.TestSuite ; + import org.slf4j.Logger ; import org.slf4j.LoggerFactory ; +import com.hp.hpl.jena.reasoner.rulesys.impl.TestLPBRuleEngineLeak; + /** * Aggregate tester that runs all the test associated with the rulesys package. */ @@ -49,6 +52,7 @@ private TestPackage() { addTest( "TestGenericRules", TestGenericRules.suite() ); addTest( "TestRETE", TestRETE.suite() ); addTest( TestSetRules.suite() ); + addTest( TestLPBRuleEngineLeak.suite() ); addTest( "OWLRuleUnitTests", OWLUnitTest.suite() ); addTest( "TestBugs", TestBugs.suite() ); addTest( "TestOWLMisc", TestOWLMisc.suite() ); From 0d3def3a08c263f3bba8bfbeb45725f18b1aa5ac Mon Sep 17 00:00:00 2001 From: Stian Soiland-Reyes Date: Sat, 21 Mar 2015 02:15:13 +0000 Subject: [PATCH 2/4] JENA-904: Propagate .close() to generator --- .../reasoner/rulesys/impl/ConsumerChoicePointFrame.java | 8 ++++++++ .../hp/hpl/jena/reasoner/rulesys/impl/FrameObject.java | 3 +++ .../hp/hpl/jena/reasoner/rulesys/impl/LPInterpreter.java | 3 ++- .../hpl/jena/reasoner/rulesys/impl/LPTopGoalIterator.java | 1 + .../reasoner/rulesys/impl/TopLevelTripleMatchFrame.java | 2 ++ 5 files changed, 16 insertions(+), 1 deletion(-) diff --git a/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/ConsumerChoicePointFrame.java b/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/ConsumerChoicePointFrame.java index fb3cac50f9b..bfe864a6ebf 100644 --- a/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/ConsumerChoicePointFrame.java +++ b/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/ConsumerChoicePointFrame.java @@ -161,6 +161,14 @@ public void setReady() { context.setReady(this); } + @Override + public void close() { + super.close(); + if (generator != null) { + generator.setComplete(); + } + } + /** * Notify that this consumer choice point has finished consuming all * the results of a closed generator. diff --git a/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/FrameObject.java b/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/FrameObject.java index f0244b3b736..a2838de8782 100644 --- a/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/FrameObject.java +++ b/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/FrameObject.java @@ -55,6 +55,9 @@ public FrameObject getLink() { * frees the frame to which this is linked. */ public void close() { + if (link != null) { + link.close(); + } } } diff --git a/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/LPInterpreter.java b/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/LPInterpreter.java index 3badd0e7936..79cb99c03d8 100644 --- a/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/LPInterpreter.java +++ b/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/LPInterpreter.java @@ -164,8 +164,9 @@ public void setTopInterpreter(LPInterpreterContext context) { */ public void close() { isComplete = true; - engine.detach(this); if (cpFrame != null) cpFrame.close(); + engine.detach(this); + } /** diff --git a/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/LPTopGoalIterator.java b/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/LPTopGoalIterator.java index 14c94e4399c..c1eb9785f1b 100644 --- a/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/LPTopGoalIterator.java +++ b/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/LPTopGoalIterator.java @@ -197,6 +197,7 @@ public void close() { // Check for any dangling generators which are complete interpreter.getEngine().checkForCompletions(); + interpreter.close(); // Close this top goal lookAhead = null; //LogFactory.getLog( getClass() ).debug( "Nulling and closing LPTopGoalIterator " + interpreter ); diff --git a/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/TopLevelTripleMatchFrame.java b/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/TopLevelTripleMatchFrame.java index e4dd8dbbc55..ab3e50068de 100644 --- a/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/TopLevelTripleMatchFrame.java +++ b/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/TopLevelTripleMatchFrame.java @@ -65,7 +65,9 @@ public boolean nextMatch(LPInterpreter interpreter) { */ @Override public void close() { + super.close(); if (matchIterator != null) matchIterator.close(); + } } From e8d8df84c0b7d23447b2cdd977d63da4e2a2b340 Mon Sep 17 00:00:00 2001 From: Stian Soiland-Reyes Date: Sat, 21 Mar 2015 02:18:35 +0000 Subject: [PATCH 3/4] JENA-904 a bit more super.close() safest to call child.close() before your own super.close --- .../jena/reasoner/rulesys/impl/TopLevelTripleMatchFrame.java | 2 +- .../com/hp/hpl/jena/reasoner/rulesys/impl/TripleMatchFrame.java | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/TopLevelTripleMatchFrame.java b/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/TopLevelTripleMatchFrame.java index ab3e50068de..d90e30abab2 100644 --- a/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/TopLevelTripleMatchFrame.java +++ b/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/TopLevelTripleMatchFrame.java @@ -65,8 +65,8 @@ public boolean nextMatch(LPInterpreter interpreter) { */ @Override public void close() { - super.close(); if (matchIterator != null) matchIterator.close(); + super.close(); } diff --git a/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/TripleMatchFrame.java b/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/TripleMatchFrame.java index dbb6f7df834..7dda4d22101 100644 --- a/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/TripleMatchFrame.java +++ b/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/TripleMatchFrame.java @@ -73,6 +73,7 @@ public boolean nextMatch(LPInterpreter interpreter) { */ @Override public void close() { if (matchIterator != null) matchIterator.close(); + super.close(); } } From 7761edcd4f4bee881f53f002e0a33d486cc43fc1 Mon Sep 17 00:00:00 2001 From: Stian Soiland-Reyes Date: Sat, 21 Mar 2015 02:30:07 +0000 Subject: [PATCH 4/4] JENA-904: close the generator.interpreter only .. so that it is removed from activeInterpreters calling .setComplete() on the other hand broke other tests as it closes too much too early --- .../reasoner/rulesys/impl/ConsumerChoicePointFrame.java | 7 +++++-- .../com/hp/hpl/jena/reasoner/rulesys/impl/FrameObject.java | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/ConsumerChoicePointFrame.java b/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/ConsumerChoicePointFrame.java index bfe864a6ebf..f479010f4a9 100644 --- a/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/ConsumerChoicePointFrame.java +++ b/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/ConsumerChoicePointFrame.java @@ -164,8 +164,11 @@ public void setReady() { @Override public void close() { super.close(); - if (generator != null) { - generator.setComplete(); + if (generator != null && generator.interpreter != null) { + generator.interpreter.close(); + // .. but NOT + //generator.setComplete(); + // as it seems to cause other tests to fail } } diff --git a/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/FrameObject.java b/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/FrameObject.java index a2838de8782..2918af2437f 100644 --- a/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/FrameObject.java +++ b/jena-core/src/main/java/com/hp/hpl/jena/reasoner/rulesys/impl/FrameObject.java @@ -54,7 +54,7 @@ public FrameObject getLink() { * Close the frame actively. This frees any internal resources, frees this frame and * frees the frame to which this is linked. */ - public void close() { + public void close() { if (link != null) { link.close(); }