-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Cancel support of operations #3890
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This reverts commit 91f1967.
…to cancel-support-attempt-2
Test Results 315 files 315 suites 1m 0s ⏱️ Results for commit add6558. ♻️ This comment has been updated with latest results. |
This comment was marked as outdated.
This comment was marked as outdated.
…mpt-2 # Conflicts: # src/main/java/graphql/execution/AbstractAsyncExecutionStrategy.java # src/main/java/graphql/execution/AsyncExecutionStrategy.java # src/main/java/graphql/execution/AsyncSerialExecutionStrategy.java # src/main/java/graphql/execution/ExecutionContext.java # src/main/java/graphql/execution/ExecutionStrategy.java # src/main/java/graphql/execution/SubscriptionExecutionStrategy.java # src/test/groovy/graphql/execution/ExecutionContextBuilderTest.groovy
private final GraphQLContext graphQLContext; | ||
@Nullable | ||
private volatile ExecutionId executionId; | ||
|
||
private final AtomicInteger isRunning = new AtomicInteger(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no longer nullable.
private volatile ExecutionId executionId; | ||
|
||
private final AtomicInteger isRunning = new AtomicInteger(0); | ||
|
||
@VisibleForTesting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
killed this - tests now create a valid one
@@ -59,6 +49,7 @@ public <U, T> CompletableFuture<U> handle(CompletableFuture<T> src, BiFunction<? | |||
if (throwable != null) { | |||
throwable = throwable.getCause(); | |||
} | |||
//noinspection DataFlowIssue | |||
return fn.apply(t, throwable); | |||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idea was giving it a yellow line
this.executionId = executionId; | ||
public void updateExecutionInput(ExecutionInput executionInput) { | ||
this.executionInput = executionInput; | ||
this.executionId = executionInput.getExecutionId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we now can change the execution input
void throwIfCancelled() throws AbortExecutionException { | ||
engineRunningState.throwIfCancelled(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are just helper methods to make it nicer inside the ExecutionStrategies
@@ -277,6 +281,8 @@ protected Object executeObject(ExecutionContext executionContext, ExecutionStrat | |||
|
|||
private BiConsumer<List<Object>, Throwable> buildFieldValueMap(List<String> fieldNames, CompletableFuture<Map<String, Object>> overallResult, ExecutionContext executionContext) { | |||
return (List<Object> results, Throwable exception) -> { | |||
exception = executionContext.possibleCancellation(exception); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does a trick - if the Throwable is already set - leave it set - otherwise check if we are cancelled and make the throwable be that
.locale(Locale.getDefault()) | ||
.engineRunningState(new EngineRunningState()) | ||
.engineRunningState(new EngineRunningState(ei)) | ||
.build() | ||
ExecutionStrategyParameters executionStrategyParameters = ExecutionStrategyParameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made it a valid EngineRunningState not a test one
…mpt-2 # Conflicts: # src/main/java/graphql/execution/EngineRunningObserver.java
…ntext wrapping mechanism
…ntext wrapping mechanism - renamed as extraordinary
…ntext wrapping mechanism - renamed as unusual
…ntext wrapping mechanism - renamed as config
…to cancel-support-attempt-2
This is related to #3879
This is the second attempt at operation cancellation.
This now uses the new
graphql.EngineRunningState
as the place to hold the ExecutionInput an check if its cancelled.The execution engine code now calls into to check
There are some helpers on execution context as well for