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/11/05 17:07:29 UTC
[PR] GH-2821: Cancel Signal unavailable without timeouts - breaks manual abort. [jena]
Aklakan opened a new pull request, #2822:
URL: https://github.com/apache/jena/pull/2822
GitHub issue resolved #2821
Pull request Description:
* Added query/update tests that register a local function that checks for the availability of the cancel signal.
* Fixed inconsistent context handling in the update builder machinery and made it consistent with that in the query builder machinery.
* The context of update requests is now forwarded to the query execution
----
- [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-2821: Cancel Signal unavailable without timeouts - breaks manual abort. [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on PR #2822:
URL: https://github.com/apache/jena/pull/2822#issuecomment-2468377446
I have consolidated the Timeout stuff (initial and overall) into the `Timeouts` class.
There is now ARQ.updateTimeout. The UpdateEngineWorker tracks its own processing time and updates its context with the remaining query time out before starting the underlying query execution.
The update timeouts are now only used to update ARQ.queryTimeout in the context.
Conversely, the update timeouts do not affect the sinks: In principle a sink could also refuse to accept changes once the timeout is reached - but if that's really needed it could be a future update.
--
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-2821: Cancel Signal unavailable without timeouts - breaks manual abort. [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on code in PR #2822:
URL: https://github.com/apache/jena/pull/2822#discussion_r1831071257
##########
jena-arq/src/main/java/org/apache/jena/http/sys/ExecUpdateHTTPBuilder.java:
##########
@@ -275,25 +276,25 @@ public Y context(Context context) {
if ( context == null )
return thisBuilder();
ensureContext();
- this.context.setAll(context);
+ this.contextAcc.context(context);
return thisBuilder();
}
public Y set(Symbol symbol, Object value) {
ensureContext();
- this.context.set(symbol, value);
+ this.contextAcc.set(symbol, value);
return thisBuilder();
}
public Y set(Symbol symbol, boolean value) {
ensureContext();
- this.context.set(symbol, value);
+ this.contextAcc.set(symbol, value);
return thisBuilder();
}
private void ensureContext() {
Review Comment:
The empty ensureContext method is akin to [ExecHTTPBuilder](https://github.com/apache/jena/blob/c7eaf83cf328496cb4f8a24b42a483199ac216c0/jena-arq/src/main/java/org/apache/jena/http/sys/ExecHTTPBuilder.java#L304). It could be removed there and in this file (`ExecUpdateHTTPBuilder`).
--
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-2821: Cancel Signal unavailable without timeouts - breaks manual abort. [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on PR #2822:
URL: https://github.com/apache/jena/pull/2822#issuecomment-2464749534
> If a global timeout for queries is set, then that will be picked up by the update execution where it wasn't 5.2 and earlier.
* One option would be to introduce `ARQ.updateTimeout` and have the UpdateEngineWorker (or perhaps already at `builder.build()`) do
```java
cxt.set(ARQ.queryTimeout, cxt.get(ARQ.updateTimeout))
```
* Furthermore, there could be an additional `ARQ.requestType` context symbol for query and update requests (and perhaps dataset ones; c.f. `LinkDataset`). And the appropriate timeout value is picked based on the context's request type which might be cleaner than having to mutate the context.
I'd say that the basic updateTimeout setting should affect the overall timeout of an update request - not per sub-update.
The suggestions should be backward compatible with the existing behavior (but may require a change for computing the effective timeout in the query exec).
--
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-2821: Support for Timeouts on Updates [jena]
Posted by "afs (via GitHub)" <gi...@apache.org>.
afs commented on code in PR #2822:
URL: https://github.com/apache/jena/pull/2822#discussion_r1846689204
##########
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterProcessBinding.java:
##########
@@ -49,7 +49,7 @@ public QueryIterProcessBinding(QueryIterator qIter, ExecutionContext context) {
nextBinding = null ;
AtomicBoolean signal;
try {
- signal = context.getContext().get(ARQConstants.symCancelQuery);
Review Comment:
Import `ARQConstants` no longer used.
##########
jena-arq/src/main/java/org/apache/jena/sparql/util/Context.java:
##########
@@ -20,13 +20,16 @@
import java.util.*;
import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.BiConsumer;
import org.apache.jena.atlas.lib.Lib;
+import org.apache.jena.atlas.logging.Log;
import org.apache.jena.query.ARQ;
import org.apache.jena.sparql.ARQConstants;
import org.apache.jena.sparql.ARQException;
import org.apache.jena.sparql.core.DatasetGraph;
+import org.apache.jena.sparql.engine.ExecutionContext;
Review Comment:
Unused import.
##########
jena-integration-tests/src/test/java/org/apache/jena/test/rdfconnection/TestRDFConnectionRemote.java:
##########
@@ -228,11 +230,27 @@ public void non_standard_syntax_update_remote_3c() {
/** Standard update syntax with substitution should work. */
@Test
public void standard_syntax_update_remote_1a() {
+ RDFNode FALSE = ResourceFactory.createTypedLiteral(false);
try ( RDFConnection conn = RDFConnectionRemote.service(server.datasetURL("/ds")).parseCheckSPARQL(false).build() ) {
conn.newUpdate()
- .update("INSERT DATA { <a> <b> <c> }")
- .update("INSERT DATA { <x> <y> <z> }")
- .substitution("foo", RDF.type).build();
+ .update("INSERT { <a> <b> <c> } WHERE { FILTER(?foo) }")
+ .update("INSERT { <x> <y> <z> } WHERE { FILTER(?foo) }")
+ .substitution("foo", FALSE)
+ .build()
+ .execute();
+ }
+ }
+
+ /** Standard update syntax with substitution should work when comments are involved. */
+ @Test
+ public void standard_syntax_update_remote_2b() {
Review Comment:
No problem having it here.
Using "rebase and merge" means the git history doesn't retain the branch structure.
--
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-2821: Cancel Signal unavailable without timeouts - breaks manual abort. [jena]
Posted by "afs (via GitHub)" <gi...@apache.org>.
afs commented on PR #2822:
URL: https://github.com/apache/jena/pull/2822#issuecomment-2462982436
This PR is a good step forward.
This PR is really two things:
* The bug about manual query execution abort
* A new feature - having timeouts on update.
The first is as little as move one line fix :-)
Having timeouts on update is good to have and the way it is done in this PR, timing out the WHERE clause of a modify `DELETE-INSERT-WHERE`, seems the best way (I'd played around with update timeout ... but stalled on that) because it protects against long-running `WHERE`.
There are various operations for updates and a general timeout on an update isn't a one change in one place. Update can be several operations all in one request so there could an overall timeout (caveat the issue of various operations).
The PR does impact on applications. This is not to say I think that it is bad (or good) idea but to draw out a consequence.
If I've read the PR with it comes down to this one line: [pass the context to the WHERE evaluation](https://github.com/apache/jena/pull/2822/files/1db78a53978eefc808851dffe730492898f85671#diff-c57bb9dc6558a713203a51ec552b8d3f0e2ac2d76a693085664beaec44cde554R540). If a global timeout for queries is set, then that will be picked up by the update execution where it wasn't 5.2 and earlier.
For Fuseki, I would guess that timeout is usually per-endpoint; it is possible to set it database or server wide as well.
For an application using Jena as a library, an application wide timeout might be present and that would impact updates with this PR.
--
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-2821: Cancel Signal unavailable without timeouts - breaks manual abort. [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on code in PR #2822:
URL: https://github.com/apache/jena/pull/2822#discussion_r1836609039
##########
jena-arq/src/main/java/org/apache/jena/sparql/exec/http/UpdateExecHTTP.java:
##########
@@ -130,13 +136,40 @@ private void executePostForm(Params thisParams) {
}
private String executeUpdate(String requestURL, BodyPublisher body, String contentType) {
- HttpRequest.Builder builder = HttpLib.requestBuilder(requestURL, httpHeaders, -1L, null);
+ HttpRequest.Builder builder = HttpLib.requestBuilder(requestURL, httpHeaders, timeout, timeoutUnit);
builder = contentTypeHeader(builder, contentType);
HttpRequest request = builder.POST(body).build();
logUpdate(updateString, request);
HttpResponse<InputStream> response = HttpLib.execute(httpClient, request);
- return handleResponseRtnString(response);
+ return HttpLib.handleResponseRtnString(response, this::setRetainedConnection);
+ }
+
+ private void setRetainedConnection(InputStream in) {
+ synchronized (cancelSignal) {
+ retainedConnection = in;
+ if (cancelSignal.get()) {
+ abort();
+ }
+ }
}
private static void logUpdate(String updateString, HttpRequest request) {}
+
+ /** Best effort that tries to close an underlying HTTP connection.
+ * May still hang waiting for the HTTP request to complete. */
+ @Override
+ public void abort() {
+ cancelSignal.set(true);
+ synchronized (cancelSignal) {
+ try {
+ InputStream in = retainedConnection;
+ if (in != null) {
+ in.close();
Review Comment:
I suppose this will just hang trying to consume any remaining data. Perhaps a test case could be created against the Fuseki server.
--
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-2821: Support for Timeouts on Updates [jena]
Posted by "afs (via GitHub)" <gi...@apache.org>.
afs commented on PR #2822:
URL: https://github.com/apache/jena/pull/2822#issuecomment-2484107778
Thank you!
--
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-2821: Support for Timeouts on Updates [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on code in PR #2822:
URL: https://github.com/apache/jena/pull/2822#discussion_r1845065044
##########
jena-integration-tests/src/test/java/org/apache/jena/test/rdfconnection/TestRDFConnectionRemote.java:
##########
@@ -228,11 +230,27 @@ public void non_standard_syntax_update_remote_3c() {
/** Standard update syntax with substitution should work. */
@Test
public void standard_syntax_update_remote_1a() {
+ RDFNode FALSE = ResourceFactory.createTypedLiteral(false);
try ( RDFConnection conn = RDFConnectionRemote.service(server.datasetURL("/ds")).parseCheckSPARQL(false).build() ) {
conn.newUpdate()
- .update("INSERT DATA { <a> <b> <c> }")
- .update("INSERT DATA { <x> <y> <z> }")
- .substitution("foo", RDF.type).build();
+ .update("INSERT { <a> <b> <c> } WHERE { FILTER(?foo) }")
+ .update("INSERT { <x> <y> <z> } WHERE { FILTER(?foo) }")
+ .substitution("foo", FALSE)
+ .build()
+ .execute();
+ }
+ }
+
+ /** Standard update syntax with substitution should work when comments are involved. */
+ @Test
+ public void standard_syntax_update_remote_2b() {
Review Comment:
Strictly speaking, this additional test case could go into a separate commit. It tests for an oversight I introduced with #2580 where update strings were joined on the semicolon without a preceding newline. In such cases, joining update strings that ended on a comment would produce syntax errors.
--
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-2821: Cancel Signal unavailable without timeouts - breaks manual abort. [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on code in PR #2822:
URL: https://github.com/apache/jena/pull/2822#discussion_r1836613661
##########
jena-arq/src/main/java/org/apache/jena/sparql/modify/UpdateEngineWorker.java:
##########
@@ -537,7 +537,7 @@ protected static Iterator<Binding> evalBindings(Query query, DatasetGraph dsg, B
}
// Not QueryExecDataset.dataset(...) because of initialBinding.
- QueryExecDatasetBuilder builder = QueryExecDatasetBuilder.create().dataset(dsg).query(query);
+ QueryExecDatasetBuilder builder = QueryExecDatasetBuilder.create().dataset(dsg).query(query).context(context);
Review Comment:
There is now an updateRemainingQueryTimeout that will set ARQ.queryTimeout based on the value of ARQ.updateTimeout and the time spent in the UpdateEngineWorker.
--
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-2821: Cancel Signal unavailable without timeouts - breaks manual abort. [jena]
Posted by "afs (via GitHub)" <gi...@apache.org>.
afs commented on code in PR #2822:
URL: https://github.com/apache/jena/pull/2822#discussion_r1832829076
##########
jena-arq/src/test/java/org/apache/jena/sparql/api/TestQueryExecutionCancel.java:
##########
@@ -229,6 +247,111 @@ public void test_cancel_json() {
cancellationTest("JSON {\":a\": \"b\"} WHERE {}", exec->exec.execJson().get(0));
}
+ /**
+ * Registers the function <urn:cancelSignal> which returns its value if present.
+ * A RuntimeException is raised if there is no cancel signal in the execution context.
+ *
+ * Note: Prior to jena-5.3.0 the cancel signal was only set when configuring a timeout.
+ * However, implementations may also want to react to manual abort via {@link QueryExec#abort()}.
+ */
+ public static FunctionRegistry registerCancelSignalFunction(FunctionRegistry fnReg) {
+ fnReg.put("urn:cancelSignal", iri -> new FunctionBase0() {
+ @Override
+ protected NodeValue exec(List<NodeValue> args, FunctionEnv env) {
+ ExecutionContext execCxt = (ExecutionContext)env;
+ AtomicBoolean cancelSignal = execCxt.getCancelSignal();
+ if (cancelSignal == null) {
+ throw new RuntimeException("No cancel signal in execution context.");
+ }
+ return NodeValue.makeBoolean(cancelSignal.get());
+ }
+
+ @Override
+ public NodeValue exec() {
+ throw new IllegalStateException("Should never be called");
+ }
+ });
+
+ return fnReg;
+ }
+
+ /** Set cancel signal function via {@link QueryExecBuilder#set(Symbol, Object). */
Review Comment:
```suggestion
/** Set cancel signal function via {@link QueryExecBuilder#set(Symbol, Object)}. */
```
##########
jena-arq/src/main/java/org/apache/jena/http/sys/ExecUpdateHTTPBuilder.java:
##########
@@ -275,25 +276,25 @@ public Y context(Context context) {
if ( context == null )
return thisBuilder();
ensureContext();
- this.context.setAll(context);
+ this.contextAcc.context(context);
return thisBuilder();
}
public Y set(Symbol symbol, Object value) {
ensureContext();
- this.context.set(symbol, value);
+ this.contextAcc.set(symbol, value);
return thisBuilder();
}
public Y set(Symbol symbol, boolean value) {
ensureContext();
- this.context.set(symbol, value);
+ this.contextAcc.set(symbol, value);
return thisBuilder();
}
private void ensureContext() {
- if ( context == null )
- context = Context.create();
+// if ( context == null )
+// context = Context.create();
Review Comment:
Remove.
Changing to `ContextAccumulator` is an improvement.
##########
jena-arq/src/main/java/org/apache/jena/sparql/modify/UpdateEngineWorker.java:
##########
@@ -537,7 +537,7 @@ protected static Iterator<Binding> evalBindings(Query query, DatasetGraph dsg, B
}
// Not QueryExecDataset.dataset(...) because of initialBinding.
- QueryExecDatasetBuilder builder = QueryExecDatasetBuilder.create().dataset(dsg).query(query);
+ QueryExecDatasetBuilder builder = QueryExecDatasetBuilder.create().dataset(dsg).query(query).context(context);
Review Comment:
See comment on the main PR. This is potentially something that impacts users and we need to consider migration.
##########
jena-arq/src/test/java/org/apache/jena/sparql/api/TestQueryExecutionCancel.java:
##########
@@ -229,6 +247,111 @@ public void test_cancel_json() {
cancellationTest("JSON {\":a\": \"b\"} WHERE {}", exec->exec.execJson().get(0));
}
+ /**
+ * Registers the function <urn:cancelSignal> which returns its value if present.
+ * A RuntimeException is raised if there is no cancel signal in the execution context.
+ *
+ * Note: Prior to jena-5.3.0 the cancel signal was only set when configuring a timeout.
+ * However, implementations may also want to react to manual abort via {@link QueryExec#abort()}.
+ */
+ public static FunctionRegistry registerCancelSignalFunction(FunctionRegistry fnReg) {
+ fnReg.put("urn:cancelSignal", iri -> new FunctionBase0() {
+ @Override
+ protected NodeValue exec(List<NodeValue> args, FunctionEnv env) {
+ ExecutionContext execCxt = (ExecutionContext)env;
+ AtomicBoolean cancelSignal = execCxt.getCancelSignal();
+ if (cancelSignal == null) {
+ throw new RuntimeException("No cancel signal in execution context.");
+ }
+ return NodeValue.makeBoolean(cancelSignal.get());
+ }
+
+ @Override
+ public NodeValue exec() {
+ throw new IllegalStateException("Should never be called");
+ }
+ });
+
+ return fnReg;
+ }
+
+ /** Set cancel signal function via {@link QueryExecBuilder#set(Symbol, Object). */
+ @Test
+ public void test_cancel_signal_query_1() {
+ DatasetGraph dsg = DatasetGraphFactory.create();
+ FunctionRegistry fnReg = registerCancelSignalFunction(new FunctionRegistry());
+ try (QueryExec qe = QueryExec.dataset(dsg).query("SELECT (<urn:cancelSignal>() AS ?foobar) { }")
+ .set(ARQConstants.registryFunctions, fnReg)
+ .build()) {
+ Assert.assertEquals(1, ResultSetFormatter.consume(ResultSet.adapt(qe.select())));
+ }
+ }
+
+ /** Set cancel signal function via {@link QueryExecBuilder#context(Context). */
Review Comment:
Fix for Javadoc warnings:
```suggestion
/** Set cancel signal function via {@link QueryExecBuilder#context(Context)}. */
```
also import `QueryExecBuilder`.
##########
jena-arq/src/main/java/org/apache/jena/sparql/exec/QueryExecDataset.java:
##########
@@ -457,6 +457,23 @@ private void startQueryIteratorActual() {
execInit();
+ // JENA-2821 - Unconditionally provide a cancel signal because manual abort via QueryExec.abort()
+ // may be triggered any time, even if no timeouts were configured.
+ // Prior to this issue, the cancel signal was only provided when timeouts were configured.
+
+ // The following note is older:
+ // JENA-2141 - the timeout can go off while building the query iterator structure.
+ // In this case, use a signal passed through the context.
+ // We don't know if getPlan().iterator() does a lot of work or not
+ // (ideally it shouldn't start executing the query but in some sub-systems
+ // it might be necessary)
+ //
+ // This applies to the time to first result because to get the first result, the
+ // queryIterator must have been built. So it does not apply for the second
+ // stage of N,-1 or N,M.
+ context.set(ARQConstants.symCancelQuery, cancelSignal);
+
+
Review Comment:
Remove one empty line.
##########
jena-arq/src/test/java/org/apache/jena/sparql/api/TestQueryExecutionCancel.java:
##########
@@ -229,6 +247,111 @@ public void test_cancel_json() {
cancellationTest("JSON {\":a\": \"b\"} WHERE {}", exec->exec.execJson().get(0));
}
+ /**
+ * Registers the function <urn:cancelSignal> which returns its value if present.
+ * A RuntimeException is raised if there is no cancel signal in the execution context.
+ *
+ * Note: Prior to jena-5.3.0 the cancel signal was only set when configuring a timeout.
+ * However, implementations may also want to react to manual abort via {@link QueryExec#abort()}.
+ */
+ public static FunctionRegistry registerCancelSignalFunction(FunctionRegistry fnReg) {
+ fnReg.put("urn:cancelSignal", iri -> new FunctionBase0() {
+ @Override
+ protected NodeValue exec(List<NodeValue> args, FunctionEnv env) {
+ ExecutionContext execCxt = (ExecutionContext)env;
+ AtomicBoolean cancelSignal = execCxt.getCancelSignal();
+ if (cancelSignal == null) {
+ throw new RuntimeException("No cancel signal in execution context.");
+ }
+ return NodeValue.makeBoolean(cancelSignal.get());
+ }
+
+ @Override
+ public NodeValue exec() {
+ throw new IllegalStateException("Should never be called");
+ }
+ });
+
+ return fnReg;
+ }
+
+ /** Set cancel signal function via {@link QueryExecBuilder#set(Symbol, Object). */
+ @Test
+ public void test_cancel_signal_query_1() {
+ DatasetGraph dsg = DatasetGraphFactory.create();
+ FunctionRegistry fnReg = registerCancelSignalFunction(new FunctionRegistry());
+ try (QueryExec qe = QueryExec.dataset(dsg).query("SELECT (<urn:cancelSignal>() AS ?foobar) { }")
+ .set(ARQConstants.registryFunctions, fnReg)
+ .build()) {
+ Assert.assertEquals(1, ResultSetFormatter.consume(ResultSet.adapt(qe.select())));
+ }
+ }
+
+ /** Set cancel signal function via {@link QueryExecBuilder#context(Context). */
+ @Test
+ public void test_cancel_signal_query_2() {
+ DatasetGraph dsg = DatasetGraphFactory.create();
+ Context cxt = ARQ.getContext().copy();
+ FunctionRegistry fnReg = registerCancelSignalFunction(new FunctionRegistry());
+ FunctionRegistry.set(cxt, fnReg);
+ try (QueryExec qe = QueryExec.dataset(dsg).query("SELECT (<urn:cancelSignal>() AS ?foobar) { }").context(cxt).build()) {
+ Assert.assertEquals(1, ResultSetFormatter.consume(ResultSet.adapt(qe.select())));
+ }
+ }
+
+ /** Set cancel signal function via {@link QueryExec#getContext()}. */
+ @Test
+ public void test_cancel_signal_query_3() {
+ DatasetGraph dsg = DatasetGraphFactory.create();
+ try (QueryExec qe = QueryExec.dataset(dsg).query("SELECT (<urn:cancelSignal>() AS ?foobar) { }").build()) {
+ FunctionRegistry fnReg = registerCancelSignalFunction(new FunctionRegistry());
+ FunctionRegistry.set(qe.getContext(), fnReg);
+ ResultSetFormatter.consume(ResultSet.adapt(qe.select()));
+ }
+ }
+
+ /** Set cancel signal function via {@link UpdateExecBuilder#set(Symbol, Object)}. */
+ @Test
+ public void test_cancel_signal_update_1() {
+ DatasetGraph dsg = DatasetGraphFactory.create();
+ FunctionRegistry fnReg = registerCancelSignalFunction(new FunctionRegistry());
+ UpdateExec ue = UpdateExec.dataset(dsg)
+ .update("INSERT { <s> <p> ?o } WHERE { BIND(<urn:cancelSignal>() AS ?o) }")
+ .set(ARQConstants.registryFunctions, fnReg)
+ .build();
+ ue.execute();
+ Assert.assertEquals(1, Iter.count(dsg.find()));
+ }
+
+ /** Set cancel signal function via {@link UpdateExecBuilder#context(Context). */
Review Comment:
```suggestion
/** Set cancel signal function via {@link UpdateExecBuilder#context(Context)}. */
```
##########
jena-arq/src/main/java/org/apache/jena/sparql/exec/QueryExecDataset.java:
##########
@@ -457,6 +457,23 @@ private void startQueryIteratorActual() {
execInit();
+ // JENA-2821 - Unconditionally provide a cancel signal because manual abort via QueryExec.abort()
+ // may be triggered any time, even if no timeouts were configured.
+ // Prior to this issue, the cancel signal was only provided when timeouts were configured.
+
+ // The following note is older:
+ // JENA-2141 - the timeout can go off while building the query iterator structure.
+ // In this case, use a signal passed through the context.
+ // We don't know if getPlan().iterator() does a lot of work or not
+ // (ideally it shouldn't start executing the query but in some sub-systems
+ // it might be necessary)
+ //
+ // This applies to the time to first result because to get the first result, the
+ // queryIterator must have been built. So it does not apply for the second
+ // stage of N,-1 or N,M.
Review Comment:
Remove.
##########
jena-arq/src/main/java/org/apache/jena/sparql/exec/http/UpdateExecHTTP.java:
##########
@@ -79,10 +79,10 @@ public static UpdateExecHTTPBuilder service(String endpointURL) {
this.sendMode = sendMode;
}
-// @Override
-// public Context getContext() {
-// return null;
-// }
+ @Override
+ public Context getContext() {
+ return context;
+ }
//
Review Comment:
Since this class is changing, please remove commented out `getDatasetGraph()`.
##########
jena-arq/src/test/java/org/apache/jena/sparql/api/TestQueryExecutionCancel.java:
##########
@@ -229,6 +247,111 @@ public void test_cancel_json() {
cancellationTest("JSON {\":a\": \"b\"} WHERE {}", exec->exec.execJson().get(0));
}
+ /**
+ * Registers the function <urn:cancelSignal> which returns its value if present.
+ * A RuntimeException is raised if there is no cancel signal in the execution context.
+ *
+ * Note: Prior to jena-5.3.0 the cancel signal was only set when configuring a timeout.
+ * However, implementations may also want to react to manual abort via {@link QueryExec#abort()}.
Review Comment:
I'm not a fan of "historical" documentation in javadoc. That just sticks around for a very long time. The metric I like is "will it be useful in 5 years time?". I don't think that passes that test.
I suggest either removing this part or putting in a non-javadoc comment.
##########
jena-arq/src/test/java/org/apache/jena/sparql/api/TestQueryExecutionCancel.java:
##########
@@ -229,6 +247,111 @@ public void test_cancel_json() {
cancellationTest("JSON {\":a\": \"b\"} WHERE {}", exec->exec.execJson().get(0));
}
+ /**
+ * Registers the function <urn:cancelSignal> which returns its value if present.
+ * A RuntimeException is raised if there is no cancel signal in the execution context.
+ *
+ * Note: Prior to jena-5.3.0 the cancel signal was only set when configuring a timeout.
+ * However, implementations may also want to react to manual abort via {@link QueryExec#abort()}.
+ */
+ public static FunctionRegistry registerCancelSignalFunction(FunctionRegistry fnReg) {
+ fnReg.put("urn:cancelSignal", iri -> new FunctionBase0() {
+ @Override
+ protected NodeValue exec(List<NodeValue> args, FunctionEnv env) {
+ ExecutionContext execCxt = (ExecutionContext)env;
+ AtomicBoolean cancelSignal = execCxt.getCancelSignal();
+ if (cancelSignal == null) {
+ throw new RuntimeException("No cancel signal in execution context.");
+ }
+ return NodeValue.makeBoolean(cancelSignal.get());
+ }
+
+ @Override
+ public NodeValue exec() {
+ throw new IllegalStateException("Should never be called");
+ }
+ });
+
+ return fnReg;
+ }
+
+ /** Set cancel signal function via {@link QueryExecBuilder#set(Symbol, Object). */
+ @Test
+ public void test_cancel_signal_query_1() {
+ DatasetGraph dsg = DatasetGraphFactory.create();
+ FunctionRegistry fnReg = registerCancelSignalFunction(new FunctionRegistry());
+ try (QueryExec qe = QueryExec.dataset(dsg).query("SELECT (<urn:cancelSignal>() AS ?foobar) { }")
+ .set(ARQConstants.registryFunctions, fnReg)
+ .build()) {
+ Assert.assertEquals(1, ResultSetFormatter.consume(ResultSet.adapt(qe.select())));
+ }
+ }
+
+ /** Set cancel signal function via {@link QueryExecBuilder#context(Context). */
+ @Test
+ public void test_cancel_signal_query_2() {
+ DatasetGraph dsg = DatasetGraphFactory.create();
+ Context cxt = ARQ.getContext().copy();
+ FunctionRegistry fnReg = registerCancelSignalFunction(new FunctionRegistry());
+ FunctionRegistry.set(cxt, fnReg);
+ try (QueryExec qe = QueryExec.dataset(dsg).query("SELECT (<urn:cancelSignal>() AS ?foobar) { }").context(cxt).build()) {
+ Assert.assertEquals(1, ResultSetFormatter.consume(ResultSet.adapt(qe.select())));
+ }
+ }
+
+ /** Set cancel signal function via {@link QueryExec#getContext()}. */
+ @Test
+ public void test_cancel_signal_query_3() {
+ DatasetGraph dsg = DatasetGraphFactory.create();
+ try (QueryExec qe = QueryExec.dataset(dsg).query("SELECT (<urn:cancelSignal>() AS ?foobar) { }").build()) {
+ FunctionRegistry fnReg = registerCancelSignalFunction(new FunctionRegistry());
+ FunctionRegistry.set(qe.getContext(), fnReg);
+ ResultSetFormatter.consume(ResultSet.adapt(qe.select()));
+ }
+ }
+
+ /** Set cancel signal function via {@link UpdateExecBuilder#set(Symbol, Object)}. */
+ @Test
+ public void test_cancel_signal_update_1() {
Review Comment:
I think these should go in new `TestUpdateExecutionCancel`.
##########
jena-arq/src/main/java/org/apache/jena/http/sys/ExecUpdateHTTPBuilder.java:
##########
@@ -275,25 +276,25 @@ public Y context(Context context) {
if ( context == null )
return thisBuilder();
ensureContext();
- this.context.setAll(context);
+ this.contextAcc.context(context);
return thisBuilder();
}
public Y set(Symbol symbol, Object value) {
ensureContext();
- this.context.set(symbol, value);
+ this.contextAcc.set(symbol, value);
return thisBuilder();
}
public Y set(Symbol symbol, boolean value) {
ensureContext();
- this.context.set(symbol, value);
+ this.contextAcc.set(symbol, value);
return thisBuilder();
}
private void ensureContext() {
Review Comment:
Yes, good idea to remove it. "Later" rarely happens.
##########
jena-arq/src/main/java/org/apache/jena/sparql/exec/UpdateExecDatasetBuilder.java:
##########
@@ -43,7 +45,11 @@ public class UpdateExecDatasetBuilder implements UpdateExecBuilder {
private DatasetGraph dataset = null;
private Query query = null;
- private Context context = null;
+ // private Context context = null;
+ private ContextAccumulator contextAcc =
+ ContextAccumulator.newBuilder(()->ARQ.getContext(), ()->Context.fromDataset(dataset));
+
+
Review Comment:
Remove one empty line.
##########
jena-arq/src/main/java/org/apache/jena/sparql/exec/QueryExecDataset.java:
##########
@@ -457,6 +457,23 @@ private void startQueryIteratorActual() {
execInit();
+ // JENA-2821 - Unconditionally provide a cancel signal because manual abort via QueryExec.abort()
+ // may be triggered any time, even if no timeouts were configured.
+ // Prior to this issue, the cancel signal was only provided when timeouts were configured.
+
+ // The following note is older:
+ // JENA-2141 - the timeout can go off while building the query iterator structure.
+ // In this case, use a signal passed through the context.
+ // We don't know if getPlan().iterator() does a lot of work or not
+ // (ideally it shouldn't start executing the query but in some sub-systems
+ // it might be necessary)
+ //
+ // This applies to the time to first result because to get the first result, the
+ // queryIterator must have been built. So it does not apply for the second
+ // stage of N,-1 or N,M.
+ context.set(ARQConstants.symCancelQuery, cancelSignal);
+
+
Review Comment:
This is, of course, the one line fix to the original issue!
--
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-2821: Cancel Signal unavailable without timeouts - breaks manual abort. [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on code in PR #2822:
URL: https://github.com/apache/jena/pull/2822#discussion_r1831071257
##########
jena-arq/src/main/java/org/apache/jena/http/sys/ExecUpdateHTTPBuilder.java:
##########
@@ -275,25 +276,25 @@ public Y context(Context context) {
if ( context == null )
return thisBuilder();
ensureContext();
- this.context.setAll(context);
+ this.contextAcc.context(context);
return thisBuilder();
}
public Y set(Symbol symbol, Object value) {
ensureContext();
- this.context.set(symbol, value);
+ this.contextAcc.set(symbol, value);
return thisBuilder();
}
public Y set(Symbol symbol, boolean value) {
ensureContext();
- this.context.set(symbol, value);
+ this.contextAcc.set(symbol, value);
return thisBuilder();
}
private void ensureContext() {
Review Comment:
The empty ensureContext method is akin to [ExecHTTPBuilder](https://github.com/apache/jena/blob/c7eaf83cf328496cb4f8a24b42a483199ac216c0/jena-arq/src/main/java/org/apache/jena/http/sys/ExecHTTPBuilder.java#L304). It could be removed there and in this file (`ExecUpdateHTTPBuilder).
--
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-2821: Support for Timeouts on Updates [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on code in PR #2822:
URL: https://github.com/apache/jena/pull/2822#discussion_r1845528581
##########
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIteratorBase.java:
##########
@@ -70,7 +70,7 @@ protected QueryIteratorBase(AtomicBoolean cancelSignal) {
}
private boolean requestingCancel() {
- return requestingCancel != null && requestingCancel.get() ;
+ return (requestingCancel != null && requestingCancel.get()) || Thread.interrupted() ;
Review Comment:
The additional interrupted check is needed in order to cleanly stop the Fuseki server in `TestSPARQLProtocolTimeout` which extends `AbstractFusekiTest`: When shutting down the Fuseki server, Jetty attempts to stop running the remaining threads via interruption. However, so far this flag was not picked up by the `QueryIterator` machinery, causing jetty to abandon the thread and log the warning from `org.eclipse.jetty.util.thread.QueuedThreadPool`:
```
17:27:10 WARN QueuedThreadPool :: Couldn't stop Thread[qtp1495161082-24,5,main]
```
--
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-2821: Support for Timeouts on Updates [jena]
Posted by "afs (via GitHub)" <gi...@apache.org>.
afs merged PR #2822:
URL: https://github.com/apache/jena/pull/2822
--
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-2821: Cancel Signal unavailable without timeouts - breaks manual abort. [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on code in PR #2822:
URL: https://github.com/apache/jena/pull/2822#discussion_r1831104076
##########
jena-arq/src/main/java/org/apache/jena/sparql/modify/UpdateEngineWorker.java:
##########
@@ -537,7 +537,7 @@ protected static Iterator<Binding> evalBindings(Query query, DatasetGraph dsg, B
}
// Not QueryExecDataset.dataset(...) because of initialBinding.
- QueryExecDatasetBuilder builder = QueryExecDatasetBuilder.create().dataset(dsg).query(query);
+ QueryExecDatasetBuilder builder = QueryExecDatasetBuilder.create().dataset(dsg).query(query).context(context);
Review Comment:
So far that context was never passed from the update engine to the query engine - so non-global custom settings (function registries, timeouts, etc) were lost here.
--
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-2821: Support for Timeouts on Updates [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on PR #2822:
URL: https://github.com/apache/jena/pull/2822#issuecomment-2483562336
I removed the two unused imports.
--
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-2821: Support for Timeouts on Updates [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on PR #2822:
URL: https://github.com/apache/jena/pull/2822#issuecomment-2482517375
I updated this PR's summary in the first post. The main changes are the introduction of `ARQ.updateTimeout` and the additional cancel check based on ` Thread.interrupted()` in `QueryIteratorBase.requestingCancel()`.
Its' ready for another review.
--
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-2821: Support for Timeouts on Updates [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on code in PR #2822:
URL: https://github.com/apache/jena/pull/2822#discussion_r1845528581
##########
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIteratorBase.java:
##########
@@ -70,7 +70,7 @@ protected QueryIteratorBase(AtomicBoolean cancelSignal) {
}
private boolean requestingCancel() {
- return requestingCancel != null && requestingCancel.get() ;
+ return (requestingCancel != null && requestingCancel.get()) || Thread.interrupted() ;
Review Comment:
The additional interrupted check is needed in order to cleanly stop the Fuseki server in `TestSPARQLProtocolTimeout` which extends `AbstractFusekiTest`: When shutting down the Fuseki server, Jetty attempts to stop the remaining threads via interruption. However, so far this flag was not picked up by the `QueryIterator` machinery, causing jetty to abandon the thread and log the warning from `org.eclipse.jetty.util.thread.QueuedThreadPool`:
```
17:27:10 WARN QueuedThreadPool :: Couldn't stop Thread[qtp1495161082-24,5,main]
```
--
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