From be6eeffa20d3813e0492b93a3c31970309569701 Mon Sep 17 00:00:00 2001 From: Tobias Kammerer Date: Sun, 23 May 2021 22:15:42 +0200 Subject: [PATCH 1/3] fix for #2068 --- .../execution/AsyncExecutionStrategy.java | 1 + ...ecutionStrategyInstrumentationContext.java | 4 + .../FieldLevelTrackingApproach.java | 7 + src/test/groovy/graphql/Issue2068.groovy | 264 ++++++++++++++++++ 4 files changed, 276 insertions(+) create mode 100644 src/test/groovy/graphql/Issue2068.groovy diff --git a/src/main/java/graphql/execution/AsyncExecutionStrategy.java b/src/main/java/graphql/execution/AsyncExecutionStrategy.java index 9c70ca06ba..3800657680 100644 --- a/src/main/java/graphql/execution/AsyncExecutionStrategy.java +++ b/src/main/java/graphql/execution/AsyncExecutionStrategy.java @@ -76,6 +76,7 @@ public CompletableFuture execute(ExecutionContext executionCont // if there are any issues with combining/handling the field results, // complete the future at all costs and bubble up any thrown exception so // the execution does not hang. + executionStrategyCtx.onFieldValuesException(); overallResult.completeExceptionally(ex); return null; }); diff --git a/src/main/java/graphql/execution/instrumentation/ExecutionStrategyInstrumentationContext.java b/src/main/java/graphql/execution/instrumentation/ExecutionStrategyInstrumentationContext.java index ff8dab082d..da11626bc3 100644 --- a/src/main/java/graphql/execution/instrumentation/ExecutionStrategyInstrumentationContext.java +++ b/src/main/java/graphql/execution/instrumentation/ExecutionStrategyInstrumentationContext.java @@ -13,4 +13,8 @@ default void onFieldValuesInfo(List fieldValueInfoList) { } + default void onFieldValuesException() { + + } + } diff --git a/src/main/java/graphql/execution/instrumentation/dataloader/FieldLevelTrackingApproach.java b/src/main/java/graphql/execution/instrumentation/dataloader/FieldLevelTrackingApproach.java index 3794d67cfb..a6a56a9500 100644 --- a/src/main/java/graphql/execution/instrumentation/dataloader/FieldLevelTrackingApproach.java +++ b/src/main/java/graphql/execution/instrumentation/dataloader/FieldLevelTrackingApproach.java @@ -156,6 +156,13 @@ public void onFieldValuesInfo(List fieldValueInfoList) { dispatch(); } } + + @Override + public void onFieldValuesException() { + synchronized (callStack) { + callStack.increaseHappenedOnFieldValueCalls(curLevel); + } + } }; } diff --git a/src/test/groovy/graphql/Issue2068.groovy b/src/test/groovy/graphql/Issue2068.groovy new file mode 100644 index 0000000000..6d5662bdb1 --- /dev/null +++ b/src/test/groovy/graphql/Issue2068.groovy @@ -0,0 +1,264 @@ +package graphql + +import graphql.execution.instrumentation.dataloader.DataLoaderDispatcherInstrumentation +import graphql.schema.DataFetcher +import graphql.schema.DataFetchingEnvironment +import graphql.schema.StaticDataFetcher +import graphql.schema.idl.RuntimeWiring +import org.apache.commons.lang3.concurrent.BasicThreadFactory +import org.dataloader.BatchLoader +import org.dataloader.DataLoader +import org.dataloader.DataLoaderOptions +import org.dataloader.DataLoaderRegistry +import spock.lang.Specification + +import java.util.concurrent.CompletableFuture +import java.util.concurrent.CompletionStage +import java.util.concurrent.SynchronousQueue +import java.util.concurrent.ThreadFactory +import java.util.concurrent.ThreadPoolExecutor +import java.util.concurrent.TimeUnit + +import static graphql.schema.idl.TypeRuntimeWiring.newTypeWiring +import static graphql.ExecutionInput.newExecutionInput + +class Issue2068 extends Specification { + def "deadlock attempt"() { + setup: + def sdl = """ + type Nation { + name: String + } + + type Toy { + name: String + } + + type Owner { + nation: Nation + } + + type Cat { + toys: [Toy] + } + + type Dog { + owner: Owner + } + + type Pets { + cats: [Cat] + dogs: [Dog] + } + + type Query { + pets: Pets + } + """ + + def cats = [['id': "cat-1"]] + def dogs = [['id': "dog-1"]] + + ThreadFactory threadFactory = new BasicThreadFactory.Builder() + .namingPattern("resolver-chain-thread-%d").build() + def executor = new ThreadPoolExecutor(15, 15, 0L, + TimeUnit.MILLISECONDS, new SynchronousQueue<>(), threadFactory, + new ThreadPoolExecutor.CallerRunsPolicy()) + + DataFetcher nationsDf = { env -> env.getDataLoader("owner.nation").load(env) } + DataFetcher ownersDf = { env -> env.getDataLoader("dog.owner").load(env) } + + def wiring = RuntimeWiring.newRuntimeWiring() + .type(newTypeWiring("Query") + .dataFetcher("pets", new StaticDataFetcher(['cats': cats, 'dogs': dogs]))) + .type(newTypeWiring("Cat") + .dataFetcher("toys", new StaticDataFetcher(new List() { + @Override + int size() { + return 1 + } + + @Override + boolean isEmpty() { + return false + } + + @Override + boolean contains(Object o) { + return false + } + + @Override + Iterator iterator() { + throw new RuntimeException() + } + + @Override + Object[] toArray() { + return new Object[0] + } + + @Override + Object[] toArray(Object[] a) { + return null + } + + @Override + boolean add(Object o) { + return false + } + + @Override + boolean remove(Object o) { + return false + } + + @Override + boolean containsAll(Collection c) { + return false + } + + @Override + boolean addAll(Collection c) { + return false + } + + @Override + boolean addAll(int index, Collection c) { + return false + } + + @Override + boolean removeAll(Collection c) { + return false + } + + @Override + boolean retainAll(Collection c) { + return false + } + + @Override + void clear() { + + } + + @Override + Object get(int index) { + return null + } + + @Override + Object set(int index, Object element) { + return null + } + + @Override + void add(int index, Object element) { + + } + + @Override + Object remove(int index) { + return null + } + + @Override + int indexOf(Object o) { + return 0 + } + + @Override + int lastIndexOf(Object o) { + return 0 + } + + @Override + ListIterator listIterator() { + return null + } + + @Override + ListIterator listIterator(int index) { + return null + } + + @Override + List subList(int fromIndex, int toIndex) { + return null + } + }))) + .type(newTypeWiring("Dog") + .dataFetcher("owner", ownersDf)) + .type(newTypeWiring("Owner") + .dataFetcher("nation", nationsDf)) + .build() + + def schema = TestUtil.schema(sdl, wiring) + + when: + def graphql = GraphQL.newGraphQL(schema) + .instrumentation(new DataLoaderDispatcherInstrumentation()) + .build() + DataLoaderRegistry dataLoaderRegistry = mkNewDataLoaderRegistry(executor) + + graphql.execute(newExecutionInput() + .dataLoaderRegistry(dataLoaderRegistry) + .query(""" + query LoadPets { + pets { + cats { + toys { + name + } + } + dogs { + owner { + nation { + name + } + } + } + } + } + """) + .build()) + + then: "execution shouldn't hang" + // wait for each future to complete and grab the results + thrown(RuntimeException) + } + + private static DataLoaderRegistry mkNewDataLoaderRegistry(executor) { + def dataLoaderNations = new DataLoader(new BatchLoader>() { + @Override + CompletionStage>> load(List keys) { + return CompletableFuture.supplyAsync({ + def nations = [] + for (int i = 1; i <= 1; i++) { + nations.add(['id': "nation-$i", 'name': "nation-$i"]) + } + return nations + }, executor) as CompletionStage>> + } + }, DataLoaderOptions.newOptions().setMaxBatchSize(5)) + + def dataLoaderOwners = new DataLoader(new BatchLoader>() { + @Override + CompletionStage>> load(List keys) { + return CompletableFuture.supplyAsync({ + def owners = [] + for (int i = 1; i <= 1; i++) { + owners.add(['id': "owner-$i"]) + } + return owners + }, executor) as CompletionStage>> + } + }, DataLoaderOptions.newOptions().setMaxBatchSize(5)) + + def dataLoaderRegistry = new DataLoaderRegistry() + dataLoaderRegistry.register("dog.owner", dataLoaderOwners) + dataLoaderRegistry.register("owner.nation", dataLoaderNations) + dataLoaderRegistry + } +} From a1b2965151e8a6bf62b932339a517e42287f0051 Mon Sep 17 00:00:00 2001 From: Tobias Kammerer Date: Mon, 24 May 2021 23:18:47 +0200 Subject: [PATCH 2/3] Adjusted test as per comment --- src/test/groovy/graphql/Issue2068.groovy | 117 ++--------------------- 1 file changed, 6 insertions(+), 111 deletions(-) diff --git a/src/test/groovy/graphql/Issue2068.groovy b/src/test/groovy/graphql/Issue2068.groovy index 6d5662bdb1..e9869119f8 100644 --- a/src/test/groovy/graphql/Issue2068.groovy +++ b/src/test/groovy/graphql/Issue2068.groovy @@ -23,7 +23,7 @@ import static graphql.schema.idl.TypeRuntimeWiring.newTypeWiring import static graphql.ExecutionInput.newExecutionInput class Issue2068 extends Specification { - def "deadlock attempt"() { + def "shouldn't hang on exception in resolveFieldWithInfo"() { setup: def sdl = """ type Nation { @@ -72,120 +72,15 @@ class Issue2068 extends Specification { .type(newTypeWiring("Query") .dataFetcher("pets", new StaticDataFetcher(['cats': cats, 'dogs': dogs]))) .type(newTypeWiring("Cat") - .dataFetcher("toys", new StaticDataFetcher(new List() { + .dataFetcher("toys", new StaticDataFetcher(new AbstractList() { @Override - int size() { - return 1 - } - - @Override - boolean isEmpty() { - return false - } - - @Override - boolean contains(Object o) { - return false - } - - @Override - Iterator iterator() { - throw new RuntimeException() - } - - @Override - Object[] toArray() { - return new Object[0] - } - - @Override - Object[] toArray(Object[] a) { - return null - } - - @Override - boolean add(Object o) { - return false - } - - @Override - boolean remove(Object o) { - return false - } - - @Override - boolean containsAll(Collection c) { - return false - } - - @Override - boolean addAll(Collection c) { - return false - } - - @Override - boolean addAll(int index, Collection c) { - return false - } - - @Override - boolean removeAll(Collection c) { - return false + Object get(int i) { + throw new RuntimeException("Simulated failure"); } @Override - boolean retainAll(Collection c) { - return false - } - - @Override - void clear() { - - } - - @Override - Object get(int index) { - return null - } - - @Override - Object set(int index, Object element) { - return null - } - - @Override - void add(int index, Object element) { - - } - - @Override - Object remove(int index) { - return null - } - - @Override - int indexOf(Object o) { - return 0 - } - - @Override - int lastIndexOf(Object o) { - return 0 - } - - @Override - ListIterator listIterator() { - return null - } - - @Override - ListIterator listIterator(int index) { - return null - } - - @Override - List subList(int fromIndex, int toIndex) { - return null + int size() { + return 1 } }))) .type(newTypeWiring("Dog") From da72a414436cacd05b17f47c283574d59118acb2 Mon Sep 17 00:00:00 2001 From: Tobias Kammerer Date: Wed, 11 May 2022 15:36:17 +0200 Subject: [PATCH 3/3] Fix silent thread leak for chained instrumentation --- .../ChainedInstrumentation.java | 4 ++ src/test/groovy/graphql/Issue2068.groovy | 37 ++++++++++++++++++- 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/src/main/java/graphql/execution/instrumentation/ChainedInstrumentation.java b/src/main/java/graphql/execution/instrumentation/ChainedInstrumentation.java index 1768a80a1a..c8df52365d 100644 --- a/src/main/java/graphql/execution/instrumentation/ChainedInstrumentation.java +++ b/src/main/java/graphql/execution/instrumentation/ChainedInstrumentation.java @@ -263,6 +263,10 @@ public void onFieldValuesInfo(List fieldValueInfoList) { contexts.forEach(context -> context.onFieldValuesInfo(fieldValueInfoList)); } + @Override + public void onFieldValuesException() { + contexts.forEach(ExecutionStrategyInstrumentationContext::onFieldValuesException); + } } } diff --git a/src/test/groovy/graphql/Issue2068.groovy b/src/test/groovy/graphql/Issue2068.groovy index e9869119f8..a111e425d1 100644 --- a/src/test/groovy/graphql/Issue2068.groovy +++ b/src/test/groovy/graphql/Issue2068.groovy @@ -1,5 +1,6 @@ package graphql +import graphql.execution.instrumentation.ChainedInstrumentation import graphql.execution.instrumentation.dataloader.DataLoaderDispatcherInstrumentation import graphql.schema.DataFetcher import graphql.schema.DataFetchingEnvironment @@ -11,6 +12,7 @@ import org.dataloader.DataLoader import org.dataloader.DataLoaderOptions import org.dataloader.DataLoaderRegistry import spock.lang.Specification +import spock.lang.Timeout import java.util.concurrent.CompletableFuture import java.util.concurrent.CompletionStage @@ -119,7 +121,40 @@ class Issue2068 extends Specification { """) .build()) - then: "execution shouldn't hang" + then: "execution with single instrumentation shouldn't hang" + // wait for each future to complete and grab the results + thrown(RuntimeException) + + when: + graphql = GraphQL.newGraphQL(schema) + .instrumentation(new ChainedInstrumentation( + Collections.singletonList(new DataLoaderDispatcherInstrumentation())) + ) + .build() + + graphql.execute(newExecutionInput() + .dataLoaderRegistry(dataLoaderRegistry) + .query(""" + query LoadPets { + pets { + cats { + toys { + name + } + } + dogs { + owner { + nation { + name + } + } + } + } + } + """) + .build()) + + then: "execution with chained instrumentation shouldn't hang" // wait for each future to complete and grab the results thrown(RuntimeException) }