You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@jena.apache.org by "Aklakan (via GitHub)" <gi...@apache.org> on 2024/12/06 21:29:23 UTC
[PR] GH-2881: Mitigate cases of open iterators when cancelling queries. [jena]
Aklakan opened a new pull request, #2882:
URL: https://github.com/apache/jena/pull/2882
GitHub issue resolved #2881
Pull request Description:
- Added a test case that should emit warnings without the fixes (the tests do not fail otherwise)
- Adds exception handling to OpUnion and StageGeneratorGeneric, mainly to cope with concurrent QueryCancelledException.
----
- [x] Tests are included.
- [x] Commits have been squashed to remove intermediate development commit messages.
- [x] Key commit messages start with the issue number (GH-xxxx)
By submitting this pull request, I acknowledge that I am making a contribution to the Apache Software Foundation under the terms and conditions of the [Contributor's Agreement](https://www.apache.org/licenses/contributor-agreements.html).
----
See the [Apache Jena "Contributing" guide](https://github.com/apache/jena/blob/main/CONTRIBUTING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2881: Mitigate cases of open iterators when cancelling queries. [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on code in PR #2882:
URL: https://github.com/apache/jena/pull/2882#discussion_r1884611607
##########
jena-arq/src/main/java/org/apache/jena/sparql/engine/main/iterator/QueryIterUnion.java:
##########
@@ -51,21 +50,51 @@ public QueryIterUnion(QueryIterator input,
@Override
protected QueryIterator nextStage(Binding binding)
{
- QueryIterConcat unionQIter = new QueryIterConcat(getExecContext()) ;
- for (Op subOp : subOps)
- {
- subOp = QC.substitute(subOp, binding) ;
- QueryIterator parent = QueryIterSingleton.create(binding, getExecContext()) ;
- QueryIterator qIter = QC.execute(subOp, parent, getExecContext()) ;
- unionQIter.add(qIter) ;
- }
-
- return unionQIter ;
+ Iterator<Op> subOpIt = subOps.iterator();
+ return new QueryIter(getExecContext()) {
+ QueryIterator qIter = null;
+
+ @Override
+ protected void requestCancel() {
+ performRequestCancel(qIter);
+ }
+
+ @Override
+ protected Binding moveToNextBinding() {
+ return qIter.next();
+ }
+
+ @Override
+ protected boolean hasNextBinding() {
+ for (;;) {
+ if (qIter != null) {
+ if (qIter.hasNext()) {
+ return true;
+ } else {
+ qIter.close();
+ qIter = null;
+ }
+ } else {
+ if (subOpIt.hasNext()) {
+ Op subOp = subOpIt.next();
+ qIter = QC.execute(subOp, binding, getExecContext());
+ } else {
+ return false;
+ }
+ }
+ }
+ }
Review Comment:
Making the nextStage() method of `QueryIterUnion` truly lazy removes the need for try-catch blocks: Sub-iterators are only opened when needed, and if a creation fails then there are no previously opened resources that need to be cleaned up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2881: Mitigate cases of open iterators when cancelling queries. [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on code in PR #2882:
URL: https://github.com/apache/jena/pull/2882#discussion_r1874387894
##########
jena-arq/src/main/java/org/apache/jena/sparql/engine/main/StageGeneratorGeneric.java:
##########
@@ -49,12 +53,29 @@ public QueryIterator execute(BasicPattern pattern, QueryIterator input, Executio
return execute(pattern, reorder, input, execCxt) ;
}
+ protected static <T> T closeOnException(Supplier<T> compute, Closeable closeAction) {
+ try {
+ T result = compute.get();
+ return result;
+ } catch (Exception e) {
+ closeAction.close();
+ e.addSuppressed(new RuntimeException("Exception during construction of basic pattern stage execution."));
+ throw e;
+ }
+ }
+
+ /**
+ * Attempts to construct an iterator that executes the input against the pattern.
+ * If the construction fails, such as due to {@link QueryCancelledException}, then the exception is passed on
+ * and the input iterator is closed.
+ */
protected QueryIterator execute(BasicPattern pattern, ReorderTransformation reorder,
QueryIterator input, ExecutionContext execCxt) {
Explain.explain(pattern, execCxt.getContext()) ;
- if ( ! input.hasNext() )
- return input ;
+ QueryIterator originalInput = input;
+ if ( closeOnException(() -> !originalInput.hasNext(), originalInput) )
+ return originalInput ;
Review Comment:
Right, I think this check is superfluous.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2881: Mitigate cases of open iterators when cancelling queries. [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on code in PR #2882:
URL: https://github.com/apache/jena/pull/2882#discussion_r1874414501
##########
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterPeek.java:
##########
@@ -25,27 +25,34 @@
public class QueryIterPeek extends QueryIter1
{
- private Binding binding = null ;
+ private Binding binding = null ;
private boolean closed = false ;
-
+
public static QueryIterPeek create(QueryIterator iterator, ExecutionContext cxt)
{
if ( iterator instanceof QueryIterPeek)
return (QueryIterPeek)iterator ;
return new QueryIterPeek(iterator, cxt) ;
}
-
+
private QueryIterPeek(QueryIterator iterator, ExecutionContext cxt)
{
super(iterator, cxt) ;
}
/** Returns the next binding without moving on. Returns "null" for no such element. */
- public Binding peek()
+ public Binding peek()
{
if ( closed ) return null ;
- if ( ! hasNextBinding() )
- return null ;
+ try {
+ if ( ! hasNextBinding() )
+ return null ;
+ } catch (Exception e) {
Review Comment:
Perhaps having the try-catch block in `StageGeneratorGeneric.execute` is slightly better. Essentially, a method that returns a QueryIterator should close its own already allocated resources early in case of a failure. It feels not totally clean having peek() deal with an exception but not hasNextBinding / moveToNext.
The thing is, that even if an iterator's hasNextBinding / moveToNext raise an exception, then this iterator will usually connected to the 'root' iterator. The query execution will catch an exception and close the root iterator which will propagate to its children and only then a check for dangling resources is done.
The problem is that StageGeneratorGeneric creates an QueryIterPeek, then calls peek which fails, and then the already registered iterator is left dangling - so its not really an issue of peek().
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2881: Mitigate cases of open iterators when cancelling queries. [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on code in PR #2882:
URL: https://github.com/apache/jena/pull/2882#discussion_r1874388989
##########
jena-arq/src/main/java/org/apache/jena/sparql/engine/main/StageGeneratorGeneric.java:
##########
@@ -65,7 +86,7 @@ protected QueryIterator execute(BasicPattern pattern, ReorderTransformation reor
QueryIterPeek peek = QueryIterPeek.create(input, execCxt) ;
// And now use this one
input = peek ;
- Binding b = peek.peek() ;
+ Binding b = closeOnException(peek::peek, peek) ;
Review Comment:
Mhm, an exception during `iter.peek()` could close `iter`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2881: Mitigate cases of open iterators when cancelling queries. [jena]
Posted by "afs (via GitHub)" <gi...@apache.org>.
afs merged PR #2882:
URL: https://github.com/apache/jena/pull/2882
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2881: Mitigate cases of open iterators when cancelling queries. [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on code in PR #2882:
URL: https://github.com/apache/jena/pull/2882#discussion_r1879147178
##########
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterPeek.java:
##########
@@ -25,27 +25,34 @@
public class QueryIterPeek extends QueryIter1
{
- private Binding binding = null ;
+ private Binding binding = null ;
private boolean closed = false ;
-
+
public static QueryIterPeek create(QueryIterator iterator, ExecutionContext cxt)
{
if ( iterator instanceof QueryIterPeek)
return (QueryIterPeek)iterator ;
return new QueryIterPeek(iterator, cxt) ;
}
-
+
private QueryIterPeek(QueryIterator iterator, ExecutionContext cxt)
{
super(iterator, cxt) ;
}
/** Returns the next binding without moving on. Returns "null" for no such element. */
- public Binding peek()
+ public Binding peek()
{
if ( closed ) return null ;
- if ( ! hasNextBinding() )
- return null ;
+ try {
+ if ( ! hasNextBinding() )
+ return null ;
+ } catch (Exception e) {
Review Comment:
I moved the try-catch block back to StageGeneratorGeneric because based on my previous argument it would be the appropriate place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2881: Mitigate cases of open iterators when cancelling queries. [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on code in PR #2882:
URL: https://github.com/apache/jena/pull/2882#discussion_r1874414501
##########
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterPeek.java:
##########
@@ -25,27 +25,34 @@
public class QueryIterPeek extends QueryIter1
{
- private Binding binding = null ;
+ private Binding binding = null ;
private boolean closed = false ;
-
+
public static QueryIterPeek create(QueryIterator iterator, ExecutionContext cxt)
{
if ( iterator instanceof QueryIterPeek)
return (QueryIterPeek)iterator ;
return new QueryIterPeek(iterator, cxt) ;
}
-
+
private QueryIterPeek(QueryIterator iterator, ExecutionContext cxt)
{
super(iterator, cxt) ;
}
/** Returns the next binding without moving on. Returns "null" for no such element. */
- public Binding peek()
+ public Binding peek()
{
if ( closed ) return null ;
- if ( ! hasNextBinding() )
- return null ;
+ try {
+ if ( ! hasNextBinding() )
+ return null ;
+ } catch (Exception e) {
Review Comment:
Perhaps having the try-catch block in `StageGeneratorGeneric.execute` is slightly better. Essentially, a method that returns a QueryIterator should close its own already allocated resources early in case of a failure. It feels not totally clean having peek() deal with an exception but not hasNextBinding / moveToNext.
The thing is, that even if an iterator's hasNextBinding / moveToNext raise an exception, then this iterator will under normal conditions be connected to the 'root' iterator. The query execution will catch an exception and close the root iterator which will propagate to its children and only then a check for dangling resources is done.
The problem is that StageGeneratorGeneric creates an QueryIterPeek, then calls peek which fails, and then the already registered iterator is left dangling - so its not really an issue of peek().
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2881: Mitigate cases of open iterators when cancelling queries. [jena]
Posted by "afs (via GitHub)" <gi...@apache.org>.
afs commented on code in PR #2882:
URL: https://github.com/apache/jena/pull/2882#discussion_r1874382764
##########
jena-arq/src/main/java/org/apache/jena/sparql/engine/main/StageGeneratorGeneric.java:
##########
@@ -49,12 +53,29 @@ public QueryIterator execute(BasicPattern pattern, QueryIterator input, Executio
return execute(pattern, reorder, input, execCxt) ;
}
+ protected static <T> T closeOnException(Supplier<T> compute, Closeable closeAction) {
+ try {
+ T result = compute.get();
+ return result;
+ } catch (Exception e) {
+ closeAction.close();
+ e.addSuppressed(new RuntimeException("Exception during construction of basic pattern stage execution."));
+ throw e;
+ }
+ }
+
+ /**
+ * Attempts to construct an iterator that executes the input against the pattern.
+ * If the construction fails, such as due to {@link QueryCancelledException}, then the exception is passed on
+ * and the input iterator is closed.
+ */
protected QueryIterator execute(BasicPattern pattern, ReorderTransformation reorder,
QueryIterator input, ExecutionContext execCxt) {
Explain.explain(pattern, execCxt.getContext()) ;
- if ( ! input.hasNext() )
- return input ;
+ QueryIterator originalInput = input;
+ if ( closeOnException(() -> !originalInput.hasNext(), originalInput) )
+ return originalInput ;
Review Comment:
The `input` should be closed in a cancel. If it is not then something is missing elsewhere.
See the issue #2881 for discussion about inverting the overall design to have explicit declaration and management of resources that have clear-up.
##########
jena-arq/src/main/java/org/apache/jena/sparql/engine/main/StageGeneratorGeneric.java:
##########
@@ -65,7 +86,7 @@ protected QueryIterator execute(BasicPattern pattern, ReorderTransformation reor
QueryIterPeek peek = QueryIterPeek.create(input, execCxt) ;
// And now use this one
input = peek ;
- Binding b = peek.peek() ;
+ Binding b = closeOnException(peek::peek, peek) ;
Review Comment:
It looks to me like `QueryIterPeek` should be updated to handle cancellations rather than putting execution costs into the main path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2881: Mitigate cases of open iterators when cancelling queries. [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on code in PR #2882:
URL: https://github.com/apache/jena/pull/2882#discussion_r1874415159
##########
jena-arq/src/main/java/org/apache/jena/sparql/engine/main/StageGeneratorGeneric.java:
##########
@@ -49,12 +53,29 @@ public QueryIterator execute(BasicPattern pattern, QueryIterator input, Executio
return execute(pattern, reorder, input, execCxt) ;
}
+ protected static <T> T closeOnException(Supplier<T> compute, Closeable closeAction) {
+ try {
+ T result = compute.get();
+ return result;
+ } catch (Exception e) {
+ closeAction.close();
+ e.addSuppressed(new RuntimeException("Exception during construction of basic pattern stage execution."));
+ throw e;
+ }
+ }
+
+ /**
+ * Attempts to construct an iterator that executes the input against the pattern.
+ * If the construction fails, such as due to {@link QueryCancelledException}, then the exception is passed on
+ * and the input iterator is closed.
+ */
protected QueryIterator execute(BasicPattern pattern, ReorderTransformation reorder,
QueryIterator input, ExecutionContext execCxt) {
Explain.explain(pattern, execCxt.getContext()) ;
- if ( ! input.hasNext() )
- return input ;
+ QueryIterator originalInput = input;
+ if ( closeOnException(() -> !originalInput.hasNext(), originalInput) )
+ return originalInput ;
Review Comment:
Removed the check again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2881: Mitigate cases of open iterators when cancelling queries. [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on PR #2882:
URL: https://github.com/apache/jena/pull/2882#issuecomment-2535912200
This actual proposed fix is complete for this PR. The test case would now also fails in case of open iterators. The number of threads created by the test case depends on the number of available cores. I am not sure whether the test case could be improved to produce the error condition more frequently.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2881: Mitigate cases of open iterators when cancelling queries. [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on code in PR #2882:
URL: https://github.com/apache/jena/pull/2882#discussion_r1884611607
##########
jena-arq/src/main/java/org/apache/jena/sparql/engine/main/iterator/QueryIterUnion.java:
##########
@@ -51,21 +50,51 @@ public QueryIterUnion(QueryIterator input,
@Override
protected QueryIterator nextStage(Binding binding)
{
- QueryIterConcat unionQIter = new QueryIterConcat(getExecContext()) ;
- for (Op subOp : subOps)
- {
- subOp = QC.substitute(subOp, binding) ;
- QueryIterator parent = QueryIterSingleton.create(binding, getExecContext()) ;
- QueryIterator qIter = QC.execute(subOp, parent, getExecContext()) ;
- unionQIter.add(qIter) ;
- }
-
- return unionQIter ;
+ Iterator<Op> subOpIt = subOps.iterator();
+ return new QueryIter(getExecContext()) {
+ QueryIterator qIter = null;
+
+ @Override
+ protected void requestCancel() {
+ performRequestCancel(qIter);
+ }
+
+ @Override
+ protected Binding moveToNextBinding() {
+ return qIter.next();
+ }
+
+ @Override
+ protected boolean hasNextBinding() {
+ for (;;) {
+ if (qIter != null) {
+ if (qIter.hasNext()) {
+ return true;
+ } else {
+ qIter.close();
+ qIter = null;
+ }
+ } else {
+ if (subOpIt.hasNext()) {
+ Op subOp = subOpIt.next();
+ qIter = QC.execute(subOp, binding, getExecContext());
+ } else {
+ return false;
+ }
+ }
+ }
+ }
Review Comment:
Making the nextStage() method of `QueryIterUnion` truly lazy removes the need for my previously proposed try-catch blocks: With this change, sub-iterators are only opened when needed, and if a creation fails then there are no previously opened resources that need to be cleaned up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org