Errors thrown while using AsyncSerialExecutionStrategy can cause stuck threads

[rlhagen]

Describe the bug
If Errors are thrown in a DataFetcher when using AsyncSerialExecutionStrategy then it's possible to craft a query that causes the thread to be stuck indefinitely.

Possible Fix
Catch Throwable here seems to give the correct behavior completing the future exceptionally which eventually hands it back to the JVM.

To Reproduce
Run the following junit test and observe that errorsShouldBeNotBeSwallowed will never terminate.

import static graphql.schema.idl.RuntimeWiring.newRuntimeWiring;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

import java.util.Arrays;
import java.util.concurrent.CompletionException;

import org.junit.Test;

import graphql.ExecutionResult;
import graphql.GraphQL;
import graphql.execution.AsyncSerialExecutionStrategy;
import graphql.schema.DataFetcher;
import graphql.schema.GraphQLSchema;
import graphql.schema.idl.RuntimeWiring;
import graphql.schema.idl.SchemaGenerator;
import graphql.schema.idl.SchemaParser;
import graphql.schema.idl.TypeDefinitionRegistry;

public class AsyncSerialExecutionStrategyTest {

	@Test(expected = CompletionException.class)
	public void errorsShouldBeNotBeSwallowed() {
		ExecutionResult executionResult = run(environment -> {
			throw new LinkageError("This error should be caught to fail the future");
		});
	}

	@Test
	public void exceptionsShouldCollected() {
		ExecutionResult executionResult = run(environment -> {
			throw new RuntimeException("This exception should be collected");
		});
		assertEquals(1, executionResult.getErrors().size());
		assertTrue(executionResult.getErrors().get(0).getMessage().contains("This exception should be collected"));
	}

	private ExecutionResult run(DataFetcher<?> fetcher) {
		String schema = "type Query{animals: [Animal] plants:[Plant]} type Animal{name: String} type Plant{name: String}";
		SchemaParser schemaParser = new SchemaParser();
		TypeDefinitionRegistry typeDefinitionRegistry = schemaParser.parse(schema);

		RuntimeWiring runtimeWiring = newRuntimeWiring()
				.type("Query", builder -> builder.dataFetcher("animals", environment -> Arrays.asList(new Object(), new Object(), new Object())))
				.type("Query", builder -> builder.dataFetcher("plants", fetcher))
				.type("Animal", builder -> builder.dataFetcher("name", environment -> "cat"))
				.type("Plant", builder -> builder.dataFetcher("name", environment -> "petunia"))
				.build();

		SchemaGenerator schemaGenerator = new SchemaGenerator();
		GraphQLSchema graphQLSchema = schemaGenerator.makeExecutableSchema(typeDefinitionRegistry, runtimeWiring);

		GraphQL build = GraphQL.newGraphQL(graphQLSchema)
				.queryExecutionStrategy(new AsyncSerialExecutionStrategy())
				.build();

		return build.execute("{animals{name} plants{name}}");
	}

}

Or add this test to AsyncSerialExecutionStrategyTest.groovy and notice the future never completes.

    def "serial execution with errors"() {
        given:
        GraphQLSchema schema = schema(
                {env -> "hello"},
                {env -> "world"},
                {env -> throw new LinkageError("This should be handled")}
        )

        String query = "{hello, hello2, hello3}"
        def document = new Parser().parseDocument(query)
        def operation = document.definitions[0] as OperationDefinition

        def typeInfo = ExecutionStepInfo.newExecutionStepInfo()
                .type(schema.getQueryType())
                .build()

        ExecutionContext executionContext = new ExecutionContextBuilder()
                .graphQLSchema(schema)
                .executionId(ExecutionId.generate())
                .operationDefinition(operation)
                .instrumentation(SimplePerformantInstrumentation.INSTANCE)
                .valueUnboxer(ValueUnboxer.DEFAULT)
                .locale(Locale.getDefault())
                .graphQLContext(GraphQLContext.getDefault())
                .build()
        ExecutionStrategyParameters executionStrategyParameters = ExecutionStrategyParameters
                .newParameters()
                .executionStepInfo(typeInfo)
                .fields(mergedSelectionSet(['hello': mergedField(new Field('hello')), 'hello2': mergedField(new Field('hello2')), 'hello3': mergedField(new Field('hello3'))]))
                .build()

        AsyncSerialExecutionStrategy strategy = new AsyncSerialExecutionStrategy()
        when:
        def result = strategy.execute(executionContext, executionStrategyParameters)
        result.isDone()
        result.get()


        then:
        final ExecutionException exception = thrown()
        exception.message == 'java.lang.LinkageError: This should be handled'
    }

[andimarek]

catching a throwable is a bit complicated thing: it normally indicates that something is seriously wrong and often you can't really recover from it. It is not a normal exception?

Did you encounter this in real life? Which exception did you encounter?

[rlhagen]

Yes, I understand that. But if the discussion is around catching Throwable... Currently, the code just crashes and it swallows the Error which is a way bigger issue than catching a Throwable. It took quite an effort to even find this issue.

We have experienced this in real life mostly with NoClassDefFoundError (could be others but can't tell because the Error is completely swallowed) due to an incorrect dependency loaded at runtime. In this case catching the Throwable does the right thing since calling join or get on the future will throw a CompletionException which is far better than just swallowing the Error and causing the Future to be stuck indefinitely.

Catching the Throwable here and completing the future exceptionally is no different than the following code.

CompletableFuture<?> future = CompletableFuture.supplyAsync(() -> {
	throw new LinkageError();
}).handle((result, throwable) -> result);
		
future.join();

java.util.concurrent.CompletionException: java.lang.LinkageError

	at java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:273)
	at java.util.concurrent.CompletableFuture.completeThrowable(CompletableFuture.java:280)
	at java.util.concurrent.CompletableFuture$AsyncSupply.run$$$capture(CompletableFuture.java:1606)
	at java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java)
	at java.util.concurrent.CompletableFuture$AsyncSupply.exec(CompletableFuture.java:1596)
	at java.util.concurrent.ForkJoinTask.doExec$$$capture(ForkJoinTask.java:289)
	at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java)
	at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)
	at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)
	at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:175)
Caused by: java.lang.LinkageError
	at graphql.CompletableFutureTest.lambda$test$0(CompletableFutureTest.java:12)
	at java.util.concurrent.CompletableFuture$AsyncSupply.run$$$capture(CompletableFuture.java:1604)
	... 7 more

[rlhagen]

@andimarek Looks like this issue was fixed with @bbakerman commit for implementing polymorphic behavior. Are you open to adding a test for the issue?

[bbakerman]

Are you open to adding a test for the issue?

Very much so - thanks