{{ message }}
Capture partial results on cancellation#4398
Open
AlexandreCarlton wants to merge 9 commits into
Open
Conversation
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.
Contributor
Test ReportTest Results
Code Coverage (Java 25)
Changed Class Coverage (4 classes)
ExecutionStrategy — method details
|
Sorry, something went wrong.
bbakerman
reviewed
May 25, 2026
bbakerman
reviewed
May 25, 2026
bbakerman
reviewed
May 25, 2026
`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
bbakerman
reviewed
May 26, 2026
bbakerman
reviewed
May 27, 2026
bbakerman
reviewed
May 27, 2026
bbakerman
approved these changes
May 27, 2026
bbakerman
left a comment
Member
There was a problem hiding this comment.
I have left some improvement comments but overall I am happy with this
Sorry, something went wrong.
andimarek
reviewed
May 27, 2026
andimarek
reviewed
May 27, 2026
andimarek
reviewed
May 27, 2026
Member
|
@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) |
Sorry, something went wrong.
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>
bbakerman
reviewed
May 29, 2026
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>
Member
|
@copilot resolve the merge conflicts in this pull request |
Sorry, something went wrong.
bbakerman
reviewed
May 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.
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.cancelwill save all the evaluatedDataFetcherresults 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.