| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
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.
Test ReportTest Results
Code Coverage (Java 25)
Changed Class Coverage (7 classes)
|
Sorry, something went wrong.
| return await(); | ||
| } | ||
|
|
||
| CompletableFuture<Void> cancellationFuture = graphQLContext.get(CANCELLATION_FUTURE_KEY); |
|
|
||
| futures.await().whenComplete((completeValueInfos, throwable) -> { | ||
| GraphQLContext graphQLContext = executionContext.getGraphQLContext(); | ||
| futures.await(graphQLContext).whenComplete((completeValueInfos, throwable) -> { |
| } | ||
|
|
||
| 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.
| def list = resultCF.join() | ||
|
|
||
| then: | ||
| list == ["X", "Y"] |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| def list = asyncBuilder.await(cancelCF).join() | ||
|
|
||
| then: | ||
| list == ["A", "B", "C"] |
There was a problem hiding this comment.
Maybe add a test with a mix of materialised and CF objects
Sorry, something went wrong.
There was a problem hiding this comment.
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?
Sorry, something went wrong.
| def list = asyncBuilder.await(cancelCF).join() | ||
|
|
||
| then: | ||
| list == ["A"] |
There was a problem hiding this comment.
Is there some condition then where if there is only 1 Cf then the cancel wont be listened to?
Should we test for that?
Sorry, something went wrong.
There was a problem hiding this comment.
Good point - have added a test for this.
Sorry, something went wrong.
|
|
||
| if (throwable != null) { | ||
| handleResultsConsumer.accept(null, throwable.getCause()); | ||
| if (capturePartialResults(executionContext) && completeValueInfos != null) { |
There was a problem hiding this comment.
maybe reverse that check completeValueInfos != null is cheaper than checking capturePartialResults(executionContext)
Sorry, something went wrong.
There was a problem hiding this comment.
I have left some improvement comments but overall I am happy with this
Sorry, something went wrong.
| // race: either all CFs complete normally, or cancellation fires first | ||
| CompletableFuture.anyOf(allOf, cancellationFuture) | ||
| .whenComplete((ignored, exception) -> { | ||
| if (overallResult.isDone()) { |
| }); | ||
|
|
||
| // also handle when allOf completes (either normally or exceptionally) after the race | ||
| allOf.whenComplete((ignored, exception) -> { |
There was a problem hiding this comment.
I don't fully understand this here: when can this happen exactly and why do we want to handle it?
Sorry, something went wrong.
|
@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>
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>
|
@copilot resolve the merge conflicts in this pull request |
Sorry, something went wrong.
| public CompletableFuture<List<T>> await(@Nullable CompletableFuture<Void> cancellationFuture) { | ||
| return await(); | ||
| } | ||
|
|
There was a problem hiding this comment.
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
Sorry, something went wrong.
| // failure always has null results). When partial capture is enabled we keep those | ||
| // results and attach the cancellation error; otherwise we report the error as usual. | ||
| if (results != null && capturePartialResults(executionContext)) { | ||
| executionContext.addError((AbortExecutionException) exception); |
There was a problem hiding this comment.
It dawns on me that it would be very useful to know which fields were still executing when we interrupted (as they or their parents were likely the reason we timed out) - but this would add a considerable amount of complexity to an already complex PR. Maybe for a future one.
Sorry, something went wrong.
Before, we were discarding the whole object / list fields instead of returning the sub-fields that had completed. Now: - `executeObject` stops collecting FieldValueInfos on cancel, rather than blocking on a slow field. - the value phase waits for cancellation-aware nested objects/list futures instead of making them `null`. - `completeValueList` preserves partial elements on cancel. We provide a `OnCancel` policy: `DROP_PENDING` for the fetch phase futures that may not complete, and `WAIT_FOR_PENDING` for futures that are cancellation-aware and thus can safely be waited on.
| Back | FazBrowse Home | New Git URL |
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.