Skip to content

onFinally() callback called too early, before the GraphQL query has finished executing #293

Closed
@lburja

Description

@lburja

Describe the bug

Due to changes made in #259, the onFinally() callback of GraphQLServletListener is called before the GraphQL query has finished executing. This affects releases 10.1.0 and 11.0.0.

More exactly, in AbstractGraphQLHttpServlet, we have the place where onFinally() callback is called:

private void doRequest(HttpServletRequest request, HttpServletResponse response) {
    List<GraphQLServletListener.RequestCallback> requestCallbacks = runListeners(
        l -> l.onRequest(request, response));

    try {
      getConfiguration().getHttpRequestHandler().handle(request, response);
      runCallbacks(requestCallbacks, c -> c.onSuccess(request, response));
    } catch (Exception t) {
      log.error("Error executing GraphQL request!", t);
      runCallbacks(requestCallbacks, c -> c.onError(request, response, t));
    } finally {
      runCallbacks(requestCallbacks, c -> c.onFinally(request, response));
    }
  }

In previous versions, the line getConfiguration().getHttpRequestHandler().handle(request, response) would block until the GraphQL query was finished. But nowadays, it just starts the query without blocking, as can be seen in HttpRequestInvokerImpl, where the execution is started using CompletableFutures, without blocking the flow:

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 {
      ...
    }
  }

(please note that under Spring Boot, request.isAsyncSupported() is by default true)

To Reproduce
Put a breakpoint (or a logging statement) in AbstractGraphQLHttpServlet at the line where onFinally() is called, and another one in HttpRequestInvokerImpl, inside the method writeResultResponse(). You'll notice that onFinally() is called before the GraphQL query has finished executing (i.e. before writeResultResponse() has been called).

Expected behavior
This is a regression. The expectation is that onFinally() is called after the GraphQL query has finished. This is useful for example, in order to release resources used by GraphQL

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions