Track Engine sync code execution by andimarek · Pull Request #3876 · graphql-java/graphql-java

andimarek · 2025-03-31T18:24:29Z

adding a counter to ExecutionContext that tracks if the engine is running

Contains a small breaking change: it makes the ExecutionContext builder non public. I think nobody should create ExecutionContexts outside GraphLQ Java.

…ning

Copilot

Pull Request Overview

This PR adds a counter to the ExecutionContext to track whether the engine is running. The key changes include:

Introducing an AtomicInteger (isRunning) with corresponding running() and finished() methods in ExecutionContext.
Inserting calls to executionContext.running() and executionContext.finished() across various execution strategy classes.
Ensuring that the execution state is tracked consistently during asynchronous execution.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/main/java/graphql/execution/SubscriptionExecutionStrategy.java	Inserts running/finished calls in the subscription strategy; includes an unnecessary semicolon that can be cleaned up.
src/main/java/graphql/execution/ExecutionStrategy.java	Adds running/finished calls to track execution state in object and field resolution flows.
src/main/java/graphql/execution/ExecutionContext.java	Introduces the AtomicInteger counter and its helper methods for tracking execution state.
src/main/java/graphql/execution/AsyncSerialExecutionStrategy.java	Adds running/finished calls around asynchronous field resolution.
src/main/java/graphql/execution/AsyncExecutionStrategy.java	Integrates running/finished calls to manage execution state during asynchronous execution.
src/main/java/graphql/execution/AbstractAsyncExecutionStrategy.java	Wraps up asynchronous result handling with running/finished calls.

src/main/java/graphql/execution/SubscriptionExecutionStrategy.java

Copilot · 2025-03-31T18:25:50Z

src/main/java/graphql/execution/SubscriptionExecutionStrategy.java

    @Override
    public CompletableFuture<ExecutionResult> execute(ExecutionContext executionContext, ExecutionStrategyParameters parameters) throws NonNullableFieldWasNullException {
-
+        executionContext.running();


Consider wrapping the code following executionContext.running() in a try-finally block to guarantee that executionContext.finished() is always called, ensuring the counter remains balanced even if exceptions occur.

Copilot · 2025-03-31T18:25:50Z

src/main/java/graphql/execution/AsyncExecutionStrategy.java

        executionStrategyCtx.onDispatched();

        futures.await().whenComplete((completeValueInfos, throwable) -> {
+            executionContext.running();


The repeated pattern of invoking executionContext.running() followed by executionContext.finished() could benefit from a refactoring (e.g. using try-finally constructs) to avoid potential imbalances in the counter during asynchronous processing.

github-actions · 2025-03-31T18:28:24Z

Test Results

313 files 313 suites 47s ⏱️
3 588 tests 3 582 ✅ 6 💤 0 ❌
3 677 runs 3 671 ✅ 6 💤 0 ❌

Results for commit 339f657.

♻️ This comment has been updated with latest results.

…java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

bbakerman · 2025-04-01T01:06:23Z

src/main/java/graphql/execution/AsyncExecutionStrategy.java

@@ -60,11 +62,13 @@ public CompletableFuture<ExecutionResult> execute(ExecutionContext executionCont
        executionStrategyCtx.onDispatched();

        futures.await().whenComplete((completeValueInfos, throwable) -> {


Is this missing a executionContext.finished() before the .await() ??

bbakerman · 2025-04-01T01:07:02Z

src/main/java/graphql/execution/AsyncExecutionStrategy.java

@@ -75,17 +79,21 @@ public CompletableFuture<ExecutionResult> execute(ExecutionContext executionCont
            dataLoaderDispatcherStrategy.executionStrategyOnFieldValuesInfo(completeValueInfos);
            executionStrategyCtx.onFieldValuesInfo(completeValueInfos);
            fieldValuesFutures.await().whenComplete(handleResultsConsumer);


executionContext.finished() before the .await() ??

bbakerman · 2025-04-01T01:07:58Z

src/main/java/graphql/execution/AsyncSerialExecutionStrategy.java

+            executionContext.finished();
            return CompletableFuture.completedFuture(isNotSensible.get());
        }



executionContext.finished() before the Async.eachSequentially ?

src/main/java/graphql/execution/ExecutionContext.java

bbakerman · 2025-04-01T01:10:46Z

src/main/java/graphql/execution/ExecutionStrategy.java

                Async.CombinedBuilder<Object> resultFutures = fieldValuesCombinedBuilder(completeValueInfos);
                dataLoaderDispatcherStrategy.executeObjectOnFieldValuesInfo(completeValueInfos, parameters);
                resolveObjectCtx.onFieldValuesInfo(completeValueInfos);
                resultFutures.await().whenComplete(handleResultsConsumer);


.await() - should this have a executionContext.finished() before it

imagine this

instead of

executionContext.finished(); resultFutures.await().whenComplete(handleResultsConsumer);

we have helper method in the engine code

awaiting(executionContext, resultFutures.await())

and in that helper method it can

call .finished()

know when the CF returns and turn itself back on

src/main/java/graphql/execution/ExecutionStrategy.java

bbakerman · 2025-04-01T01:15:42Z

src/main/java/graphql/execution/SubscriptionExecutionStrategy.java

@@ -69,19 +69,24 @@ public CompletableFuture<ExecutionResult> execute(ExecutionContext executionCont
        //
        // when the upstream source event stream completes, subscribe to it and wire in our adapter
        CompletableFuture<ExecutionResult> overallResult = sourceEventStream.thenApply((publisher) -> {


Do we need to turn this off here ??

bbakerman · 2025-04-01T01:16:36Z

src/main/java/graphql/execution/SubscriptionExecutionStrategy.java

+            executionContext.running();
            Object publisher = fetchedValue.getFetchedValue();
            if (publisher != null) {
                assertTrue(publisher instanceof Publisher, () -> "Your data fetcher must return a Publisher of events when using graphql subscriptions");


If this threw an exception - we would still be running

bbakerman · 2025-04-01T23:12:29Z

src/main/java/graphql/execution/ExecutionContext.java

+        isRunning.incrementAndGet();
+        try {
+            return callable.call();
+        } catch (Exception e) {


why catch Exception - this seems wrong.

A GraphqlAbort exception for example will be wrapped in RuntimeException and its meaning lost

Ahh Callable throws exception - perhaps make a RuntimeCallable class instead

bbakerman · 2025-04-01T23:13:03Z

src/main/java/graphql/execution/ExecutionContext.java

+        try {
+            runnable.run();
+            ;
+        } catch (Exception e) {


Runnable does not throw exceptions - so dont catch it

naming

bbakerman · 2025-04-02T06:38:46Z

src/main/java/graphql/execution/ExecutionContext.java

    }
+
+    @Nullable
+    EngineRunningObserver getEngineRunningObserver() {


@Internal

bbakerman · 2025-04-02T06:39:10Z

src/main/java/graphql/execution/ExecutionContextBuilder.java

 import static graphql.collect.ImmutableKit.emptyList;

-@PublicApi
+@Internal


Breaking change

Yep ... for 23

adding a counter to ExecutionContext that tracks if the engine is run…

a224614

…ning

andimarek requested a review from Copilot March 31, 2025 18:24

Copilot AI reviewed Mar 31, 2025

View reviewed changes

Update src/main/java/graphql/execution/SubscriptionExecutionStrategy.…

18fa1fa

…java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

bbakerman reviewed Apr 1, 2025

View reviewed changes

src/main/java/graphql/execution/ExecutionContext.java Show resolved Hide resolved

bbakerman reviewed Apr 1, 2025

View reviewed changes

src/main/java/graphql/execution/ExecutionStrategy.java Show resolved Hide resolved

bbakerman reviewed Apr 1, 2025

View reviewed changes

use util method

7d5308a

andimarek mentioned this pull request Apr 1, 2025

Added ExecutionInput cancellation #3880

Merged

dondonz added this to the 23.1 non-breaking changes and bugfixes milestone Apr 1, 2025

bbakerman reviewed Apr 1, 2025

View reviewed changes

andimarek and others added 3 commits April 2, 2025 11:37

fix exception handling,

75fe340

naming

Added tests for EC is running

a2e42f3

add an observer for the running state

82c4ef3

andimarek added the breaking change requires a new major version to be relased label Apr 2, 2025

andimarek modified the milestones: 23.1 non-breaking changes and bugfixes, 23.0 breaking changes Apr 2, 2025

bbakerman reviewed Apr 2, 2025

View reviewed changes

src/main/java/graphql/execution/ExecutionContext.java

}

@Nullable

EngineRunningObserver getEngineRunningObserver() {

Copy link

Member

bbakerman Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Internal

bbakerman reviewed Apr 2, 2025

View reviewed changes

increase accuracy of engine running

339f657

bbakerman approved these changes Apr 2, 2025

View reviewed changes

bbakerman merged commit 5a03541 into master Apr 2, 2025
2 checks passed

		@@ -60,11 +62,13 @@ public CompletableFuture<ExecutionResult> execute(ExecutionContext executionCont
		executionStrategyCtx.onDispatched();

		futures.await().whenComplete((completeValueInfos, throwable) -> {

Comments

Conversation

andimarek commented Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Mar 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

andimarek commented Mar 31, 2025 •

edited

Loading

github-actions bot commented Mar 31, 2025 •

edited

Loading