GH-2821: Support for Timeouts on Updates #2822

Aklakan · 2024-11-05T17:07:27Z

GitHub issue resolved #2821

Pull request Description:

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
UpdateEngineWorker sets the remaining query timeout if the newly introduced ARQ.updateTimeout has been set. Note, that it is possible to set an initial timeout via the ARQ.updateTimeout from which internally an ARQ.queryTimeout setting is derived: An update request would abort if the corresponding query execution timed out on the initial timeout - so only updates with a WHERE clause are affected.
I did not add initialTimeout methods to the update exec builders because perhaps initial timeout on updates are a bit to specific for the general interface - opinions?
Added UpdateProcessor.getContext() for consistency with QueryExec. This is for discussion.
Added UpdateProcessor.abort() which can abort the underlying query execution if it is based on QueryIterator.
Added test cases to the TestUpdateExecutionCancel class.
For HTTP-based updates, the overall update timeout is forwarded to the existing HTTP machinery.
There is now a class TestSPARQLProtocolTimeout which tests setting the timeout on the update builder.
QueryIteratorBase.requestingCancel was updated to check the Thread.interrupted() so that when Fuseki gets shut down (e.g. during unit testing) then any threads still busy executing queries can cancel execution in a timely manner.
Added query/update tests that register a local function that checks for the availability of the cancel signal. Please also see the test cases which register the function that checks for the cancel signal in three ways for both queries and updates:
1. via query/update builder.set
2. via query/update builder.context
3. via query/update exec.getContext()

Tests are included.
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.

See the Apache Jena "Contributing" guide.

Aklakan · 2024-11-06T14:01:47Z

jena-arq/src/main/java/org/apache/jena/http/sys/ExecUpdateHTTPBuilder.java

        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() {


The empty ensureContext method is akin to ExecHTTPBuilder. It could be removed there and in this file (ExecUpdateHTTPBuilder).

Yes, good idea to remove it. "Later" rarely happens.

Aklakan · 2024-11-06T14:22:13Z

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);


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.

See comment on the main PR. This is potentially something that impacts users and we need to consider migration.

There is now an updateRemainingQueryTimeout that will set ARQ.queryTimeout based on the value of ARQ.updateTimeout and the time spent in the UpdateEngineWorker.

afs · 2024-11-07T18:49:26Z

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. 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.

jena-arq/src/test/java/org/apache/jena/sparql/api/TestQueryExecutionCancel.java

afs · 2024-11-07T15:16:14Z

jena-arq/src/main/java/org/apache/jena/http/sys/ExecUpdateHTTPBuilder.java

        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() {


Yes, good idea to remove it. "Later" rarely happens.

jena-arq/src/main/java/org/apache/jena/http/sys/ExecUpdateHTTPBuilder.java

jena-arq/src/main/java/org/apache/jena/sparql/exec/UpdateExecDatasetBuilder.java

jena-arq/src/main/java/org/apache/jena/sparql/exec/http/UpdateExecHTTP.java

jena-arq/src/test/java/org/apache/jena/sparql/api/TestQueryExecutionCancel.java

afs · 2024-11-07T15:28:43Z

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);


See comment on the main PR. This is potentially something that impacts users and we need to consider migration.

Aklakan · 2024-11-08T13:17:44Z

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 updateExecBuilder.build()) do

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.e. no need to copy the updateTimeout value to queryTimeout.

I'd say that the basic updateTimeout setting should affect the overall timeout of an update request - not per sub-update - so the remaining time would have to be passed on to the query exec. Hm, in this case the context would have to be mutated or copied.

The suggestions should be backward compatible with the existing behavior (but may require a change for computing the effective timeout in the query exec).

Aklakan · 2024-11-11T12:37:10Z

jena-arq/src/main/java/org/apache/jena/sparql/exec/http/UpdateExecHTTP.java

+            try {
+                InputStream in = retainedConnection;
+                if (in != null) {
+                    in.close();


I suppose this will just hang trying to consume any remaining data. Perhaps a test case could be created against the Fuseki server.

Aklakan · 2024-11-11T14:58:02Z

I have consolidated the timeout logic (initial and overall) into the Timeouts class.
There is now ARQ.updateTimeout. The UpdateEngineWorker tracks its own elapsed 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.
If no update timeout is set then the UpdateEngineWorker will unset the query timeout which should be consistent with the previous behavior.

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.

Aklakan · 2024-11-16T16:40:15Z

...egration-tests/src/test/java/org/apache/jena/test/rdfconnection/TestRDFConnectionRemote.java

+
+    /** Standard update syntax with substitution should work when comments are involved. */
+    @Test
+    public void standard_syntax_update_remote_2b() {


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.

No problem having it here.
Using "rebase and merge" means the git history doesn't retain the branch structure.

Aklakan · 2024-11-17T16:53:43Z

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() ;


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]

Aklakan · 2024-11-18T10:08:24Z

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().

It's ready for another review.

afs

LGTM.
A couple of unused imports which I can sort out after the merge.

jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterProcessBinding.java

afs · 2024-11-18T14:31:31Z

jena-arq/src/main/java/org/apache/jena/sparql/util/Context.java

 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;


Unused import.

afs · 2024-11-18T14:36:11Z

...egration-tests/src/test/java/org/apache/jena/test/rdfconnection/TestRDFConnectionRemote.java

+
+    /** Standard update syntax with substitution should work when comments are involved. */
+    @Test
+    public void standard_syntax_update_remote_2b() {


No problem having it here.
Using "rebase and merge" means the git history doesn't retain the branch structure.

Aklakan · 2024-11-18T16:41:30Z

I removed the two unused imports.

afs · 2024-11-18T20:53:15Z

Thank you!

Aklakan force-pushed the 2024-11-05_cancel-signal-fix branch from fa6b5bf to 3f50e50 Compare November 5, 2024 17:10

afs self-assigned this Nov 5, 2024

Aklakan commented Nov 6, 2024

View reviewed changes

Aklakan force-pushed the 2024-11-05_cancel-signal-fix branch 2 times, most recently from dd2ef56 to 1db78a5 Compare November 6, 2024 14:08

Aklakan commented Nov 6, 2024

View reviewed changes

afs reviewed Nov 7, 2024

View reviewed changes

Aklakan marked this pull request as draft November 10, 2024 21:22

Aklakan force-pushed the 2024-11-05_cancel-signal-fix branch 6 times, most recently from f433c17 to 375244b Compare November 11, 2024 12:34

Aklakan commented Nov 11, 2024

View reviewed changes

Aklakan force-pushed the 2024-11-05_cancel-signal-fix branch 2 times, most recently from 2dcfb44 to e1e54e2 Compare November 11, 2024 14:52

Aklakan marked this pull request as ready for review November 11, 2024 14:53

Aklakan changed the title ~~GH-2821: Cancel Signal unavailable without timeouts - breaks manual abort.~~ GH-2821: Support for Timeouts on Updates Nov 11, 2024

Aklakan force-pushed the 2024-11-05_cancel-signal-fix branch 3 times, most recently from 9c84c57 to d9cbb32 Compare November 16, 2024 16:31

Aklakan commented Nov 16, 2024

View reviewed changes

Aklakan force-pushed the 2024-11-05_cancel-signal-fix branch 2 times, most recently from d8ae6d9 to b276f97 Compare November 17, 2024 16:44

Aklakan commented Nov 17, 2024

View reviewed changes

afs approved these changes Nov 18, 2024

View reviewed changes

apacheGH-2821: Support for abort and timeouts on updates.

ef354a5

Aklakan force-pushed the 2024-11-05_cancel-signal-fix branch from b276f97 to ef354a5 Compare November 18, 2024 16:39

afs merged commit 6313965 into apache:main Nov 18, 2024

Aklakan mentioned this pull request Nov 24, 2024

Added documentation for arq:updateTimeout. apache/jena-site#203

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-2821: Support for Timeouts on Updates #2822

GH-2821: Support for Timeouts on Updates #2822

Aklakan commented Nov 5, 2024 •

edited

Loading

Aklakan Nov 6, 2024 •

edited

Loading

afs Nov 7, 2024

Aklakan Nov 6, 2024

afs Nov 7, 2024

Aklakan Nov 11, 2024

afs commented Nov 7, 2024

afs Nov 7, 2024

afs Nov 7, 2024

Aklakan commented Nov 8, 2024 •

edited

Loading

Aklakan Nov 11, 2024

Aklakan commented Nov 11, 2024 •

edited

Loading

Aklakan Nov 16, 2024

afs Nov 18, 2024

Aklakan Nov 17, 2024 •

edited

Loading

Aklakan commented Nov 18, 2024 •

edited

Loading

afs left a comment

afs Nov 18, 2024

afs Nov 18, 2024

Aklakan commented Nov 18, 2024

afs commented Nov 18, 2024

GH-2821: Support for Timeouts on Updates #2822

GH-2821: Support for Timeouts on Updates #2822

Conversation

Aklakan commented Nov 5, 2024 • edited Loading

Aklakan Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afs commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aklakan commented Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Aklakan commented Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aklakan Nov 17, 2024 • edited Loading

Choose a reason for hiding this comment

Aklakan commented Nov 18, 2024 • edited Loading

afs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aklakan commented Nov 18, 2024

afs commented Nov 18, 2024

Aklakan commented Nov 5, 2024 •

edited

Loading

Aklakan Nov 6, 2024 •

edited

Loading

Aklakan commented Nov 8, 2024 •

edited

Loading

Aklakan commented Nov 11, 2024 •

edited

Loading

Aklakan Nov 17, 2024 •

edited

Loading

Aklakan commented Nov 18, 2024 •

edited

Loading