Chanqes to QueryIterSort -- ensure that when a QueryCancelledException
is thrown through SortedBindingIterator, it closes the parent
QueryIterSort.


diff --git
a/jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterSort.java
b/jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterSort.java
index 173da34..04c2655 100644
---
a/jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterSort.java
+++
b/jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterSort.java
@@ -92,6 +92,7 @@ public class QueryIterSort extends QueryIterPlainWrapper {
             // iterator in a try/finally block, and thus will call
             // close() themselves.
             catch (QueryCancelledException e) {
+               QueryIterSort.this.close();
                 close();
                 throw e;
             }

Changes to TestQueryIterSort: three tests; one to ensure that
directly close()ing the QueryIterSort also closes the underlying
source iterator; one to ensure that when the QueryIterSort is
run to completion, the source iterator is self-closed; and one to
ensure that when an exception is thrown part-way through
running the QueryIterSort, the source iterator is closed.

Plus changes to CallbackIterator to track whether close has
been called and to make it self-closing:


diff --git
a/jena-arq/src/test/java/org/apache/jena/sparql/engine/iterator/TestQueryIterSort.java
b/jena-arq/src/test/java/org/apache/jena/sparql/engine/iterator/TestQueryIterSort.java
index dd1b08c..1f78a7f 100644
---
a/jena-arq/src/test/java/org/apache/jena/sparql/engine/iterator/TestQueryIterSort.java
+++
b/jena-arq/src/test/java/org/apache/jena/sparql/engine/iterator/TestQueryIterSort.java
@@ -18,9 +18,7 @@

 package org.apache.jena.sparql.engine.iterator;

-import static org.junit.Assert.assertEquals ;
-import static org.junit.Assert.assertNotNull ;
-import static org.junit.Assert.assertTrue ;
+import static org.junit.Assert.*;

 import java.util.ArrayList ;
 import java.util.Iterator ;
@@ -133,6 +131,37 @@ public class TestQueryIterSort {
         }

         assertEquals(0, DataBagExaminer.countTemporaryFiles(qIter.db)) ;
+    }
+
+    @Test public void testCloseClosesSourceIterator() {
+        Context context = new Context() ;
+        ExecutionContext ec = new ExecutionContext(context, (Graph) null,
(DatasetGraph) null, (OpExecutorFactory) null);
+        QueryIterSort qis = new QueryIterSort(iterator, comparator, ec);
+        qis.close();
+        assertTrue("source iterator should have been closed",
iterator.isClosed());
+    }
+
+    @Test public void testExhaustionClosesSourceIterator() {
+        iterator.setCallback(new Callback()
+            { @Override public void call() { /* do nothing */ }
+        });
+        Context context = new Context() ;
+        ExecutionContext ec = new ExecutionContext(context, (Graph) null,
(DatasetGraph) null, (OpExecutorFactory) null);
+        QueryIterSort qis = new QueryIterSort(iterator, comparator, ec);
+        while (qis.hasNext()) qis.next();
+        assertTrue("source iterator should have been closed",
iterator.isClosed());
+    }
+
+    @Test public void testCancelClosesSourceIterator() {
+        Context context = new Context() ;
+        ExecutionContext ec = new ExecutionContext(context, (Graph) null,
(DatasetGraph) null, (OpExecutorFactory) null);
+        QueryIterSort qis = new QueryIterSort(iterator, comparator, ec);
+        try {
+            while (qis.hasNext()) qis.next();
+            fail("query should have been cancelled by trigger");
+        } catch (QueryCancelledException q) {
+            assertTrue("source iterator should have been closed",
iterator.isClosed());
+        }
     }

     @Test
@@ -286,6 +315,7 @@ public class TestQueryIterSort {
         int trigger ;
         Iterator<Binding> delegate ;
         boolean canceled = false ;
+        boolean closed = false;

         public CallbackIterator(Iterator<Binding> delegate, int trigger,
Callback callback)
         {
@@ -294,7 +324,7 @@ public class TestQueryIterSort {
             this.trigger = trigger ;
         }

-        public void setCallback(Callback callback)
+        public void setCallback(Callback callback)
         {
             this.callback = callback ;
         }
@@ -302,7 +332,10 @@ public class TestQueryIterSort {
         @Override
         public boolean hasNext()
         {
-            return delegate.hasNext() ;
+            // self-closing
+            boolean has = delegate.hasNext() ;
+            if (has == false) closed = true;
+            return has ;
         }

         @Override
@@ -325,6 +358,10 @@ public class TestQueryIterSort {
         {
             return elementsReturned ;
         }
+
+        public boolean isClosed() {
+            return closed;
+        }

         public boolean isCanceled() {
             return canceled ;
@@ -341,7 +378,7 @@ public class TestQueryIterSort {
         public void cancel() { canceled = true ; }

         @Override
-        public void close() { }
+        public void close() { closed = true ; }

         @Override
         public void output(IndentedWriter out, SerializationContext sCxt)
{ throw new ARQNotImplemented() ;

Chris


On 11 April 2017 at 14:48, Chris Dollin <[email protected]>
wrote:

> working on those now ...
>
> On 11 April 2017 at 14:30, afs <[email protected]> wrote:
>
>> Github user afs commented on the issue:
>>
>>     https://github.com/apache/jena/pull/236
>>
>>     This PR covers the "Bug in SortedDataBag?" report as well.  Waiting
>> on tests for that.
>>
>>
>> ---
>> If your project is set up for it, you can reply to this email and have
>> your
>> reply appear on GitHub as well. If your project does not have this feature
>> enabled and wishes so, or if the feature is enabled but not working,
>> please
>> contact infrastructure at [email protected] or file a JIRA ticket
>> with INFRA.
>> ---
>>
>
>
>
> --
> "What I don't understand is this ..."   Trevor Chaplin, /The Beiderbeck
> Affair/
>
> Epimorphics Ltd, http://www.epimorphics.com
> Registered address: Court Lodge, 105 High Street, Portishead, Bristol BS20
> 6PT
> Epimorphics Ltd. is a limited company registered in England (number
> 7016688)
>



-- 
"What I don't understand is this ..."   Trevor Chaplin, /The Beiderbeck
Affair/

Epimorphics Ltd, http://www.epimorphics.com
Registered address: Court Lodge, 105 High Street, Portishead, Bristol BS20
6PT
Epimorphics Ltd. is a limited company registered in England (number 7016688)

Reply via email to