GH-2881: Mitigate cases of open iterators when cancelling queries. #2882
Conversation
Aklakan
force-pushed
the
2024-12-06-cancel-leak
branch
from
54440a4 to
cf30474
Compare
| @@ -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) ; | |||
There was a problem hiding this comment.
It looks to me like QueryIterPeek should be updated to handle cancellations rather than putting execution costs into the main path.
There was a problem hiding this comment.
Mhm, an exception during iter.peek() could close iter.
jena-arq/src/main/java/org/apache/jena/sparql/engine/main/StageGeneratorGeneric.java
Outdated
Show resolved
Hide resolved
Aklakan
force-pushed
the
2024-12-06-cancel-leak
branch
4 times, most recently
from
db77f93 to
9f1564a
Compare
| try { | ||
| if ( ! hasNextBinding() ) | ||
| return null ; | ||
| } catch (Exception e) { |
There was a problem hiding this 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().
There was a problem hiding this comment.
I moved the try-catch block back to StageGeneratorGeneric because based on my previous argument it would be the appropriate place.
Aklakan
force-pushed
the
2024-12-06-cancel-leak
branch
4 times, most recently
from
7221065 to
e9293d1
Compare
|
This actual proposed fix is complete for this PR. The test case would now also fail 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 - so far adding more cores increases the chance. |
Aklakan
force-pushed
the
2024-12-06-cancel-leak
branch
from
e9293d1 to
e153b53
Compare
| 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; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this 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.
Aklakan
force-pushed
the
2024-12-06-cancel-leak
branch
2 times, most recently
from
d6e4a2e to
bc317ce
Compare
Aklakan
force-pushed
the
2024-12-06-cancel-leak
branch
from
bc317ce to
de0e047
Compare
GitHub issue resolved #2881
Pull request Description:
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.
See the Apache Jena "Contributing" guide.