Capture partial results on cancellation by AlexandreCarlton · Pull Request #4398 · graphql-java/graphql-java

AlexandreCarlton · 2026-05-24T11:51:05Z

Web clients can often impose request timeouts. If a GraphQL server takes too long to respond, then nothing is returned. While we can call ExecutionInput.cancel() in order to slide under a web client's timeout, there are no usable results for the client to handle.

This change improves upon this - if we set #capturePartialResultsOnCancel, then calling .cancel will save all the evaluated DataFetcher results so that something potentially usable can be returned to the client.

Full disclosure: almost all of this was generated with Rovo, with a preliminary review from @bbakerman before submitting here.

Web clients can often impose request timeouts. If a GraphQL server takes too long to respond, then nothing is returned. While we can call `ExecutionInput.cancel()` in order to slide a web client's timeout, there are no usable results for the client to use. This change improves on this - if we set `#capturePartialResultsOnCancel`, then calling `.cancel` will save all the evaluated `DataFetcher` results so that something potentially usable can be returned to the client. Full disclosure: almost all of this was generated with Rovo, with a preliminary review from Brad before submitting here.

github-actions · 2026-05-24T12:00:09Z

Test Report

Test Results

Java Version	Total	Passed	Failed	Errors	Skipped
Java 11	5885 (+4 🟢)	5829 (+4 🟢)	0 (±0)	0 (±0)	56 (±0)
Java 17	5885 (+4 🟢)	5828 (+4 🟢)	0 (±0)	0 (±0)	57 (±0)
Java 21	5885 (+4 🟢)	5828 (+4 🟢)	0 (±0)	0 (±0)	57 (±0)
Java 25	5885 (+4 🟢)	5828 (+4 🟢)	0 (±0)	0 (±0)	57 (±0)
jcstress	32 (±0)	32 (±0)	0 (±0)	0 (±0)	0 (±0)
Total	23572 (+16 🟢)	23345 (+16 🟢)	0 (±0)	0 (±0)	227 (±0)

Code Coverage (Java 25)

Metric	Covered	Missed	Coverage	vs Master
Lines	29715	3132	90.5%	±0.0%
Branches	8754	1532	85.1%	±0.0%
Methods	7931	1210	86.8%	±0.0%

Changed Class Coverage (4 classes)

Class	Line	Branch	Method
g.e.AbstractAsyncExecutionStrategy	+5.6% 🟢	±0.0%	+5.0% 🟢
g.e.Execution	+0.8% 🟢	+4.8% 🟢	±0.0%
g.e.ExecutionStrategy	-0.6% 🔴	±0.0%	+0.1% 🟢
g.GraphQLUnusualConfiguration $CancellationConfig	+80.0% 🟢	±0.0%	+66.7% 🟢

ExecutionStrategy — method details

Method	Line	Branch
capturePartialResults new	100.0%	—
getCancellationFuture new	100.0%	100.0%
completeFieldValueMap new	100.0%	—
lambda$buildFieldValueMap$0	75.0% (-25.0% 🔴)	90.0% (-10.0% 🔴)
lambda$executeObject$0 new	100.0%	100.0%
~~lambda$executeObject$0~~	removed	removed

Full HTML report: build artifact jacoco-html-report

Updated: 2026-05-29 08:03:18 UTC

Sign in to view

+                return await();
+            }
+
+            CompletableFuture<Void> cancellationFuture = graphQLContext.get(CANCELLATION_FUTURE_KEY);


Sign in to view


-        futures.await().whenComplete((completeValueInfos, throwable) -> {
+        GraphQLContext graphQLContext = executionContext.getGraphQLContext();
+        futures.await(graphQLContext).whenComplete((completeValueInfos, throwable) -> {


Sign in to view

+            }
+
+            CompletableFuture<Void> cancellationFuture = graphQLContext.get(CANCELLATION_FUTURE_KEY);
+            if (cancellationFuture == null) {


`Async` is intended to be a simple utility class - we thus just pass in the `CompletableFuture`s that are necessary to have this function.

AlexandreCarlton · 2026-05-27T11:35:29Z

Sign in to view

+        def list = resultCF.join()
+
+        then:
+        list == ["X", "Y"]


The idea was that we still get our results out even if the cancelCF hasn't completed. I've fixed up the test to clarify this.

bbakerman · 2026-05-26T23:49:20Z

+        def list = asyncBuilder.await(cancelCF).join()
+
+        then:
+        list == ["A", "B", "C"]


Maybe add a test with a mix of materialised and CF objects

I think we had this with await with cancelCF returns partial results with mixed objects and CFs - are you thinking of a different kind of test case?

bbakerman · 2026-05-27T00:05:36Z

+        def list = asyncBuilder.await(cancelCF).join()
+
+        then:
+        list == ["A"]


Is there some condition then where if there is only 1 Cf then the cancel wont be listened to?

Should we test for that?

Good point - have added a test for this.

bbakerman · 2026-05-27T00:06:36Z


            if (throwable != null) {
-                handleResultsConsumer.accept(null, throwable.getCause());
+                if (capturePartialResults(executionContext) && completeValueInfos != null) {


maybe reverse that check completeValueInfos != null is cheaper than checking capturePartialResults(executionContext)

bbakerman

I have left some improvement comments but overall I am happy with this

Sign in to view

+            // race: either all CFs complete normally, or cancellation fires first
+            CompletableFuture.anyOf(allOf, cancellationFuture)
+                    .whenComplete((ignored, exception) -> {
+                        if (overallResult.isDone()) {


andimarek · 2026-05-27T20:19:32Z

Sign in to view

+                    });
+
+            // also handle when allOf completes (either normally or exceptionally) after the race
+            allOf.whenComplete((ignored, exception) -> {


I don't fully understand this here: when can this happen exactly and why do we want to handle it?

andimarek · 2026-05-27T20:21:06Z

@AlexandreCarlton In general we need the 100% code coverage to proof that all new code is actually reachable (because AI is great in inventing unnecessary defensive logic and checks)

Collapse the two whenComplete callbacks into a single one on anyOf(allOf, cancellationFuture). The previous second callback on allOf was unreachable dead code - anyOf already fires when either future completes - and the overallResult.isDone() guards could never be true, hurting branch coverage. Merge harvestPartialResults and collectAllResults into a single harvestResults helper: when allOf has won the race every field future is done so it yields the full result, and when cancellation wins it yields the partial result with null for not-yet-done futures. join() is safe because allOf being incomplete implies no field future has failed. All new code is now 100% line and branch covered. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

The exception != null branch tried to capture partial results, but a whenComplete((value, throwable)) callback always delivers a null value when a throwable is present, so the results != null guard was never true and that capture branch was unreachable (JaCoCo confirmed zero coverage). The exception path now simply delegates to handleNonNullException. Also drop two dead CompletionException unwraps - possibleCancellation returns a raw AbortExecutionException, never a wrapped one - and the always-true instanceof/results != null checks on the cancellation path. No behavioural change (full suite passes); the method is now 100% line and branch covered. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

handleResultsWithPartialData differed from handleResults in exactly one case: a cancellation that fired after some fields completed shows up as a synthesised AbortExecutionException with a non-null results list (a real field failure always carries null results). Fold that single case into handleResults via a `results != null && capturePartialResults` check and delete handleResultsWithPartialData. In AsyncExecutionStrategy.execute the partial-results-on-cancel branch and the normal branch both built field values, fired the instrumentation callbacks and awaited the same way - only differing in tolerating null FieldValueInfo entries. Extract that into completeFieldValues, used by both paths, shrinking the execute() lambda back to roughly its pre-feature shape. The instrumentation list is only copied/filtered when null entries are actually present, so the common path stays allocation-free. Also drop a redundant CompletionException unwrap (handleNonNullException already unwraps) and the now-unused imports. Add a test for the previously-untested behaviour where, with capture disabled, a late cancel discards even fully gathered nested data. All changed code is 100% line and branch covered; full suite passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

bbakerman · 2026-05-29T07:41:47Z

@copilot resolve the merge conflicts in this pull request

bbakerman · 2026-05-29T07:50:25Z

+        public CompletableFuture<List<T>> await(@Nullable CompletableFuture<Void> cancellationFuture) {
+            return await();
+        }
+


Sorry this is a bad merge via the web editor and now its not compiling.

I fixed locally but I cant push back to Alexandre branch it seems

bbakerman reviewed May 25, 2026

View reviewed changes

Comment thread src/main/java/graphql/execution/Async.java Outdated

return await();

}

CompletableFuture<Void> cancellationFuture = graphQLContext.get(CANCELLATION_FUTURE_KEY);

This comment was marked as resolved.

Sign in to view

bbakerman reviewed May 25, 2026

View reviewed changes

Comment thread src/main/java/graphql/execution/Async.java Outdated

}

CompletableFuture<Void> cancellationFuture = graphQLContext.get(CANCELLATION_FUTURE_KEY);

if (cancellationFuture == null) {

This comment was marked as outdated.

Sign in to view

Lift out GraphQLContext from Async

6247698

`Async` is intended to be a simple utility class - we thus just pass in the `CompletableFuture`s that are necessary to have this function.

bbakerman reviewed May 26, 2026

View reviewed changes

bbakerman reviewed May 27, 2026

View reviewed changes

bbakerman approved these changes May 27, 2026

View reviewed changes

Flesh out test cases fro AsyncTest

4dd9643

andimarek reviewed May 27, 2026

View reviewed changes

Comment thread src/main/java/graphql/execution/AsyncExecutionStrategy.java Outdated

andimarek reviewed May 27, 2026

View reviewed changes

bbakerman reviewed May 29, 2026

View reviewed changes

Comment thread src/main/java/graphql/execution/Async.java

andimarek and others added 2 commits May 29, 2026 10:43

bbakerman and others added 2 commits May 29, 2026 17:43

Merge branch 'master' into capture-partial-results-on-cancel

fd1fba6

Oops bad merge via web

fecf007

bbakerman reviewed May 29, 2026

View reviewed changes

Conversation

AlexandreCarlton commented May 24, 2026

Uh oh!

github-actions Bot commented May 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Report

Test Results

Code Coverage (Java 25)

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as resolved.

Uh oh!

AlexandreCarlton May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbakerman May 26, 2026

Choose a reason for hiding this comment

Uh oh!

AlexandreCarlton May 27, 2026

Choose a reason for hiding this comment

Uh oh!

bbakerman May 27, 2026

Choose a reason for hiding this comment

Uh oh!

AlexandreCarlton May 27, 2026

Choose a reason for hiding this comment

Uh oh!

bbakerman May 27, 2026

Choose a reason for hiding this comment

Uh oh!

bbakerman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

andimarek May 27, 2026

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

andimarek commented May 27, 2026

Uh oh!

Uh oh!

bbakerman commented May 29, 2026

Uh oh!

bbakerman May 29, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented May 24, 2026 •

edited

Loading

AlexandreCarlton May 27, 2026 •

edited

Loading