HttpRequestInvokerImpl swallows Exception that prevents sending the error description to client e.g., for timeout error

[msingh-ms]

Describe the bug
HttpRequestInvokerImpl swallows Exception that prevents sending the error description to client e.g., for timeout error

try {
GraphQLQueryResult result = invoke(invocationInput, request, response).join();
writeResultResponse(invocationInput, result, request, response);
} catch (Exception t) {
writeErrorResponse(t, response);
}

To Reproduce
Steps to reproduce the behavior:

Define a Timeout Instrumentation as follows:

private static class MaxQueryTimeInstrumentation extends SimpleInstrumentation
{
    // Time limit in milliseconds
    private long maxQueryTime = 5000;

    public MaxQueryTimeInstrumentation(long maxQueryTime)
    {
        this.maxQueryTime = maxQueryTime;
    }

    private static class TimerInstrumentationState implements InstrumentationState
    {
        private Instant start = Instant.now();

        private boolean test(long maxQueryTime)
        {
            return ChronoUnit.MILLIS.between(start, Instant.now()) > maxQueryTime;
        }
    }

    @Override
    public InstrumentationState createState()
    {
        return new MaxQueryTimeInstrumentation.TimerInstrumentationState();
    }

    @Override
    public InstrumentationContext<Object> beginFieldFetch(InstrumentationFieldFetchParameters parameters)
    {
        if (((MaxQueryTimeInstrumentation.TimerInstrumentationState) parameters.getInstrumentationState())
                .test(maxQueryTime))
            throw new GraphQLException("allocated time limit exceeded");

        return super.beginFieldFetch(parameters);
    }
}

Run a query that times out.

Expected behavior
If request times out then there will be Exception based on timeout and we should be able to let client know that request timed out.

Screenshots

[oliemansm]

@msingh00021 Could you throw an AbortExecutionException instead of this generic GraphQLException and try again? That exception is also used for other security related instrumentations, such as max query depth, and is caught and converted into an error response.

[msingh-ms]

@oliemansm AbortExecutionException only works when the data fetching process has not started. If, lets say 25% of data is retrieved and request times out then there will be 200 OK response with only data section without any error section in payload

[oliemansm]

Converting exceptions thrown from the graphql-java ecosystem into proper GraphQL responses is the responsibility of the graphql-java library. I've done some digging around, and it seems that handling exceptions thrown from instrumentations has been a known issue for a long time already (2017), see graphql-java/graphql-java#460.

I've looked into several ways using the current version (16.2) to see if there's a way to recognize that exception being thrown and manipulate the ExecutionResult. For example by looking into the InstrumentationContext.onCompleted callback. The ExecutionResult interface does not provide any means to add any errors to it if it doesn't contain any errors already unfortunately.

[oliemansm]

@msingh00021 I've implemented something for this after all. Will be part of 11.1.1. Error response will be a status OK and body in line with the exception that was thrown:

{
  "errors": [
    {
      "message": "allocated time limit exceeded",
      "locations": []
    }
  ],
  "data": null
}

[msingh-ms]

@oliemansm, Thank you so much!