Support Async Timeout Property in GraphQLServletProperties

[Thinkenterprise]

Would it be possible to add the attribute asyncTimeout in the GraphQLServletProperties so that the `GraphQLWebAutoConfigurationcan load it to theGraphQLConfiguration``? So that it is possible to set a request timeout for each query. This was introduced in GraphQL Servlet in 9.2, but cannot be configured in Spring Boot. However, that would be very helpful to ward off malicious queries.

[theozaurus]

Thanks for this change, I stumbled upon this while trying to set the same thing. I'm in the process of trying to test the effect of the timeout, but I can't actually get it to timeout. I can't see any reference to this in the documentation, and looking at the code I can't see where this value gets used.

Could someone point me in the right direction?

[Thinkenterprise]

I hope this helps you further?
The property is set at the end of the code snippet.

public class GraphQLWebAutoConfiguration {
... 
@Bean
  @ConditionalOnMissingBean
  public GraphQLConfiguration graphQLServletConfiguration(
      GraphQLInvocationInputFactory invocationInputFactory,
      GraphQLInvoker graphQLInvoker,
      GraphQLObjectMapper graphQLObjectMapper,
      @Autowired(required = false) List<GraphQLServletListener> listeners,
      @Autowired(required = false) BatchInputPreProcessor batchInputPreProcessor,
      @Autowired(required = false) GraphQLResponseCacheManager responseCacheManager) {
    return GraphQLConfiguration.with(invocationInputFactory)
        .with(graphQLInvoker)
        .with(graphQLObjectMapper)
        .with(listeners)
        .with(graphQLServletProperties.getSubscriptionTimeout())
        .with(batchInputPreProcessor)
        .with(graphQLServletProperties.getContextSetting())
        .with(responseCacheManager)
        .asyncTimeout(graphQLServletProperties.getAsyncTimeout())
        .build();
  }

[theozaurus]

Thanks @Thinkenterprise. I've got it being set there correctly, I just can't observe any change in behaviour. I'm not sure if there is something else I need to do.

[dhitalsand]

I am using graphql.servlet.async-timeout: properties in springboot project to set a request timeout.
But the error message that I am getting when the request time exceeds the limit defined in properties file is

{
  "timestamp": "2021-05-06T19:05:25.134+0000",
  "status": 500,
  "error": "Internal Server Error",
  "message": "Server Error",
  "path": "/graphql"
}

which does not accurately describe that this is a timeout error.
Is there a way to change error message ?

[oliemansm]

Try setting graphql.servlet.exception-handlers-enabled to true and try again.

[Thinkenterprise]

@theozaurus I have the same problem. I set the async-timeout to 5000 and make a Thred.sleep(40000) in my resolvers. But it has no effect. I debug the source code an find that the AsyncContext will be set to the right timeout. The exception handlers are activated exception-handlers-enabled: true.

@Override
  public void execute(GraphQLInvocationInput invocationInput, HttpServletRequest request,
      HttpServletResponse response) {
    if (request.isAsyncSupported()) {
      AsyncContext asyncContext = request.isAsyncStarted()
          ? request.getAsyncContext()
          : request.startAsync(request, response);
      asyncContext.setTimeout(configuration.getAsyncTimeout());
      invoke(invocationInput, request, response)
          .thenAccept(result -> writeResultResponse(invocationInput, result, request, response))
          .exceptionally(t -> writeErrorResponse(t, response))
          .thenAccept(aVoid -> asyncContext.complete());
    } else {
      try {
        GraphQLQueryResult result = invoke(invocationInput, request, response).join();
        writeResultResponse(invocationInput, result, request, response);
      } catch (Exception t) {
        writeErrorResponse(t, response);
      }
    }
  }

After 40000 ms I get the result an no timout erros occurse!??

@dhitalsand It is a mystery to me why it works for you!?

@oliemansm Do you know who is monitoring the timeout?

[Thinkenterprise]

@oliemansm @theozaurus I have an idea why the timeout has no effect. There is no listener that catches the timeout event and produces a corresponding cancellation and error message. Can this be?

public interface AsyncListener extends EventListener {
    void onComplete(AsyncEvent event) throws IOException;
    void onTimeout(AsyncEvent event) throws IOException;
    void onError(AsyncEvent event) throws IOException;
    void onStartAsync(AsyncEvent event) throws IOException;
}

[oliemansm]

@Thinkenterprise I think the AsyncContext is actually useless here icw that CompletableFuture. Instead we'll probably have to replace that timeout with the acceptEither approach as described here: http://iteratrlearning.com/java9/2016/09/13/java9-timeouts-completablefutures.html

[Thinkenterprise]

Yes, I think the solution you suggested is also possible. However, the solution has the disadvantage that the timeout is no longer initiated by the servlet container, but by us. When we use an AsyncListener, the timeout comes from the servlet container. However, we would then have to stop the CompletableFuture via cancel (), which is not that easy, right? But as I said, I am also for the solution you proposed. I believe that this is the cleanest solution!!

[oliemansm]

@Thinkenterprise @dhitalsand
The problem was that the timeout was set on the AsyncContext, but then the CompletableFuture was not started in a runnable on that AsyncContext, but instead was just executed on the main thread. So it was not enforced. It now correctly returns a response when it times out. Status code 200 with following body:

{
  "errors": [
    {
      "message": "Timeout",
      "locations": []
    }
  ],
  "data": null
}

If you have any suggestions for a better or clearer message, now is the time.

I've also tried to cancel the GraphQL future responsible for actually running the query. Unfortunately that future appears to be blocking surprisingly, and that's because of something in the graphql-java library. I'll clean up the code and leave the possibility to have it cancel the GraphQL future in there. That way when that future from graphql-java is no longer blocking and can be canceled, this library is already able to cope with it.

Would be much better if we not only abort the request, but also can cancel the actual graphql execution. Otherwise I guess the server would still be vulnerable to a DOS attack by firing these queries that timeout, but still try to finish running in the background.

[oliemansm]

@Thinkenterprise See PR: graphql-java-kickstart/graphql-java-servlet#342

[Thinkenterprise]

@oliemansm I understand the problem and have already thought about it. But I think we now have an acceptable solution. That was still a lot that you only had to change because of the timeout, thank you !! If you still ask about the error message: It would be great if you could also put the context information such as path or location in the error. But I'm not sure whether you have access to this information at this point? In addition, the information which timeout took effect, i.e. the number of seconds or milliseconds, would be very helpful for the client. That would be the cream on the cake :-)

[oliemansm]

@Thinkenterprise I was actually mistaken about graphql-java being blocking. It was because the example I was using was a bit too simple and didn't properly create CompletableFuture compatible DataFetcher. See this explanation: graphql-java/graphql-java#2289 (reply in thread).

Verified that I can in fact now cancel the graphql execution, but that means some further refactoring to make it all work properly.

I can't include context information like location in the error, since the timeout happens outside of the graphql execution, so there's no information included from graphql-java at that time unfortunately. I'll include the timing information though, that's easy.

[oliemansm]

@Thinkenterprise

{
  "errors": [
    {
      "message": "Execution canceled because timeout of 1000 millis was reached",
      "locations": []
    }
  ],
  "data": null
}

[Thinkenterprise]

@oliemansm I already thought that it would not be easy to get the context information. Therefore the error message is completely ok. I understood about the CompletableStage etc. I think the refactoring effort is too high for the benefit. You can put that on another release. From my point of view, the issue can be closed.

[oliemansm]

@Thinkenterprise i actually already refactored it, so it's already supported now if you use the 11.1.1-SNAPSHOT version of GraphQL-Java-servlet

[oliemansm]

Release in 11.1.0