You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@jena.apache.org by "goutamadwant (via GitHub)" <gi...@apache.org> on 2026/09/14 07:29:13 UTC
[PR] GH-3837: Stop SERVICE materialization when query cancellation is observed [jena]
goutamadwant opened a new pull request, #4223:
URL: https://github.com/apache/jena/pull/4223
Refs #3837
Pull request Description:
Check the outer query's cancellation signal before executing a SERVICE request and while materializing its results. When cancellation is observed, abort the HTTP query and close its response without starting another potentially blocking read.
Successful responses remain fully materialized, preserving SERVICE SILENT error handling and variable remapping. The existing HTTP timeout configuration is unchanged.
Tests cover cancellation before a request, during result consumption, and after a blocked read returns; repeated abort/close; detached results; SILENT behavior; variable remapping; and timeout propagation.
Validation: all nine new regressions, the full ARQ reactor, 97 HTTP integration tests, and 12 Fuseki-main service access tests passed. The integration build skipped UI tooling/tests, Javadocs, and RAT; this is not a full-project verification claim.
This does not interrupt a network read already in progress or address all the Fuseki thread and connection exhaustion factors discussed in #3837. A local execution of the reported workload still stalled after client timeouts. This is a partial cancellation improvement, not a fix for the entire incident, and is not intended to close the issue.
----
- [x] Tests are included.
- [ ] Documentation change and updates are provided for the [Apache Jena website](https://github.com/apache/jena-site/)
- [ ] Commits have been squashed to remove intermediate development commit messages.
- [ ] 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-3837: Stop SERVICE materialization when query cancellation is observed [jena]
Posted by "rvesse (via GitHub)" <gi...@apache.org>.
rvesse commented on code in PR #4223:
URL: https://github.com/apache/jena/pull/4223#discussion_r4003939563
##########
jena-arq/src/main/java/org/apache/jena/sparql/exec/http/Service.java:
##########
@@ -241,9 +246,20 @@ public static QueryIterator exec(OpService op, Context context) {
.sendMode(querySendMode)
.build()) {
- // Detach from the network stream.
- RowSet rowSet = qExec.select().materialize();
- QueryIterator qIter = QueryIterPlainWrapper.create(rowSet);
+ // Detach from the network stream, but stop consuming on outer query cancellation.
+ // This cannot interrupt a network read that is already in progress.
+ checkCancelled(cancelSignal, qExec);
+ RowSet rowSet = qExec.select();
+ List<Binding> rows = new ArrayList<>();
+ for (;;) {
+ checkCancelled(cancelSignal, qExec);
+ boolean hasNext = rowSet.hasNext();
+ checkCancelled(cancelSignal, qExec);
Review Comment:
I am always a little wary of doing checks like this twice inside a loop although I can understand the reasoning (checking both before and after the network read which could block), might it make more sense to make this a `while` loop conditional on the cancel signal? Only other change that would necessitate would be a final cancellation check post-loop as `hasNext` could be `false` leading to break out of the loop before the next cancellation check
For the case when no cancel signal is available it might also be simpler to preserve existing code path from deleted lines then the actual cancellation check doesn't need to make a `null` check as well
--
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-3837: Stop SERVICE materialization when query cancellation is observed [jena]
Posted by "goutamadwant (via GitHub)" <gi...@apache.org>.
goutamadwant commented on code in PR #4223:
URL: https://github.com/apache/jena/pull/4223#discussion_r4011195080
##########
jena-arq/src/test/java/org/apache/jena/sparql/exec/TS_ExecSPARQL.java:
##########
@@ -29,6 +29,7 @@
TestExecEnvironment.class
, TestQueryExecDataset.class
, TestQueryExecution.class
+ , org.apache.jena.sparql.exec.http.TestServiceCancellation.class
Review Comment:
sure @rvesse updated to use an import and the short class name, matching the surrounding suite.
--
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-3837: Stop SERVICE materialization when query cancellation is observed [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on PR #4223:
URL: https://github.com/apache/jena/pull/4223#issuecomment-5699750924
I now refined/update my previous comment and tested against slightly modified tests of this PR according to description.
--
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-3837: Stop SERVICE materialization when query cancellation is observed [jena]
Posted by "goutamadwant (via GitHub)" <gi...@apache.org>.
goutamadwant commented on code in PR #4223:
URL: https://github.com/apache/jena/pull/4223#discussion_r4011199879
##########
jena-arq/src/main/java/org/apache/jena/sparql/exec/http/Service.java:
##########
@@ -241,9 +246,20 @@ public static QueryIterator exec(OpService op, Context context) {
.sendMode(querySendMode)
.build()) {
- // Detach from the network stream.
- RowSet rowSet = qExec.select().materialize();
- QueryIterator qIter = QueryIterPlainWrapper.create(rowSet);
+ // Detach from the network stream, but stop consuming on outer query cancellation.
+ // This cannot interrupt a network read that is already in progress.
+ checkCancelled(cancelSignal, qExec);
+ RowSet rowSet = qExec.select();
+ List<Binding> rows = new ArrayList<>();
+ for (;;) {
+ checkCancelled(cancelSignal, qExec);
+ boolean hasNext = rowSet.hasNext();
+ checkCancelled(cancelSignal, qExec);
Review Comment:
Thanks @rvesse. restoredc the original materialization path when no cancel signal is available and moved the cancellation guards into a `while` condition. The final check also catches cancellation when `hasNext()` returns false. Kept the guards around the potentially blocking read and added coverage for cancellation at the end of results and successful results with and without a signal. let me know if this looks good
--
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-3837: Stop SERVICE materialization when query cancellation is observed [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on PR #4223:
URL: https://github.com/apache/jena/pull/4223#issuecomment-5777306540
I see that my proposal for wrapping the materialization into a QueryIter has already been incorporated into to PR.
@afs, @rvesse: Are there any general objections to this approach - i.e. perhaps you can think of consequences which I overlooked?
Other than that, I'll try to complete a review of this within the next week.
--
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-3837: Stop SERVICE materialization when query cancellation is observed [jena]
Posted by "rvesse (via GitHub)" <gi...@apache.org>.
rvesse commented on code in PR #4223:
URL: https://github.com/apache/jena/pull/4223#discussion_r4003922361
##########
jena-arq/src/test/java/org/apache/jena/sparql/exec/TS_ExecSPARQL.java:
##########
@@ -29,6 +29,7 @@
TestExecEnvironment.class
, TestQueryExecDataset.class
, TestQueryExecution.class
+ , org.apache.jena.sparql.exec.http.TestServiceCancellation.class
Review Comment:
Why fully qualified class name here? Better to match existing code style and add an appropriate `import` statement
--
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-3837: Stop SERVICE materialization when query cancellation is observed [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on PR #4223:
URL: https://github.com/apache/jena/pull/4223#issuecomment-5676723685
Hi, My feeling is that it would be better to defer materialization into its own iterator.
This way, the existing cancellation/closing machinery should be reused, and cancellation might even disconnect from the HTTP connection. I have not yet tested this code against @goutamadwant tests though.
https://github.com/Aklakan/jena/commit/c77aa778210f1fd163d21c5e0c93aedbed3a05bd
In summary:
```java
// Service.java
QueryExec qExec = ...;
// Build and access the rowSet here because
// some tests expect Service.exec to raise a QueryExceptionHTTP immediately.
// If the tests could be relaxed then this prior check could be removed.
RowSet rowSet;
try {
rowSet = qExec.select();
rowSet.hasNext();
} catch (Throwable t) {
try {
if (t instanceof HttpException ex) {
throw QueryExceptionHTTP.rewrap(ex);
}
throw t;
} finally {
qExec.close();
}
}
QueryIterator qIter = new QueryIterMaterializeQueryExec(execCxt, qExec, rowSet);
if (requiresRemapping)
qIter = QueryIter.map(qIter, varMapping);
```
```java
// QueryIterMaterializeQueryExec.java
/** QueryIter backed by a QueryExec. The RowSet is materialized on the first call to next(). */
public class QueryIterMaterializeQueryExec
extends QueryIter
{
protected final QueryExec queryExec;
protected RowSet baseRowSet;
protected RowSet materializeRowSet;
/**
* Constructor.
*
* @param execCxt ExecutionContext. May be null, however cancellation signals are passed via contexts.
* @param queryExec QueryIter. The backing query execution. Must not be null.
* @param baseRowSet The backing row set. Must not be null.
*/
public QueryIterMaterializeQueryExec(ExecutionContext execCxt, QueryExec queryExec, RowSet baseRowSet) {
super(execCxt);
this.queryExec = queryExec;
this.baseRowSet = Objects.requireNonNull(baseRowSet);
}
@Override
protected boolean hasNextBinding() {
try {
if (materializeRowSet == null) {
return baseRowSet.hasNext();
}
return materializeRowSet.hasNext();
} catch (HttpException ex) {
throw QueryExceptionHTTP.rewrap(ex);
}
}
@Override
protected Binding moveToNextBinding() {
try {
if (materializeRowSet == null) {
materializeRowSet = baseRowSet.materialize();
}
return materializeRowSet.next();
} catch (HttpException ex) {
throw QueryExceptionHTTP.rewrap(ex);
}
}
@Override
protected void closeIterator() {
queryExec.close();
}
@Override
protected void requestCancel() {
queryExec.abort();
}
}
```
--
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