Provide a WebGraphQlInterceptor that supports HTTP timeouts

[tarun3300]

Hi,

We would like to setup custom timeout for the async GraphQL query like we used to do in the kickstart GraphQL as below.

#Graphql query timeout settings
graphql.servlet.async.timeout=5s

[rstoyanchev]

Is your goal to control malicious queries, as mentioned in this issue? A simple timeout is a very basic mechanism that can impact other scenarios as well such as a slow network that have nothing to do with that concern.

GraphQL Java provides Query Complexity and Query Depth instrumentations. We think that's a better mechanism to control malicious queries. There is a Boot issue #469 for an enhancement in that area, and we can consider improved support here as well if there are concrete suggestions.

That said if you really wanted, you can apply a timeout through an interceptor. For example:

@Bean
WebGraphQlInterceptor interceptor() {
	return (request, chain) -> chain.next(request)
			.timeout(Duration.ofSeconds(2), Mono.error(new ResponseStatusException(HttpStatus.SERVICE_UNAVAILABLE)));
}

This ends the response and should also cancel GraphQL request processing. However, note there is currently an issue that prevents it from working. You can watch reactor/reactor-core#3138, and when that's fixed, it should work as expected.

[spring-projects-issues]

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

[spring-projects-issues]

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.

[turboezh]

@rstoyanchev Imho, special property for global request timeout is the first thing anyone would try to lookup. Reactor way is very non-obvious. Could it be reasonable to built in and autoconfigure such an interceptor based on corresponding property?

[amruta98]

Hello @rstoyanchev, is there any update on this. I have tried above solution but it is not working

[rstoyanchev]

My apologies, indeed the above doesn't work when data is fetched synchronously because blocking occurs as part of the initial call while composing the reactive chain.

I've confirmed the following works:

public class RequestTimeoutInterceptor implements WebGraphQlInterceptor {

	private final Duration timeout;

	public RequestTimeoutInterceptor(Duration timeout) {
		this.timeout = timeout;
	}

	@Override
	public Mono<WebGraphQlResponse> intercept(WebGraphQlRequest request, Chain chain) {
		return chain.next(request)
				.subscribeOn(Schedulers.boundedElastic())
				.timeout(this.timeout, Mono.error(new ResponseStatusException(HttpStatus.SERVICE_UNAVAILABLE)));
	}

}

I am re-opening this in order to make such an interceptor available. Using an interceptor has the advantage of targeting GraphQL request handling vs any other HTTP request.

[amruta98]

Hi @rstoyanchev, Thanks for reopening this issue. I tried the interceptor option but it didn't work. The query that I am working on has following structure:

query getData {
  appData {
    name
    appConfig{
      name
      id
    }
  }
}

Now appConfig resolver's batch loader is taking too much time to fetch data because of which whole query gets aborted after 30 seconds with AsyncRequestTimeoutException.

[rstoyanchev]

@amruta98 can you double check that the interceptor is registered and is getting invoked, e.g. by adding a log message or putting a breakpoint? It should be as simple as declaring it as a bean but just to be sure.

[bclozel]

Thanks @tstocker-black-cape , this is enough to reproduce. I managed to isolate this behavior in a minimal sample and it appears the signal is lost somewhere in GraphQL. I have started a discussion on graphql-java as I'm not sure about the expected behavior here.

[bclozel]

I was mistaken, it seems this is a well-known limitation of CompletableFuture and there's nothing we can do about it here.
Now we should consider whether providing a timeout interceptor in spring-graphql is relevant still because of this behavior.

[tstocker-black-cape]

Buuuumer... Thanks for looking into this. Here's a minimal example of our workaround. I'm not sure if you'd be interested in integrating this into spring boot. There may be edge cases we're missing, but this works for our use cases.

// The interceptor configures the timeout on the mono and manually passes state to a sink that is stored as a context value
/**
 * Interceptor to set the request timeout
 */
@Configuration
public class TimeoutInterceptor implements WebGraphQlInterceptor {
	// Constants
	private static final String IS_CANCELLED_CONTEXT_KEY = "isCancelled";
	private static final Duration REQUEST_TIMEOUT = Duration.ofSeconds(10);

	@NonNull
	@Override
	public Mono<WebGraphQlResponse> intercept(
			@NonNull WebGraphQlRequest request,
			@NonNull Chain chain) {
		// Create the is cancelled flag
		Sinks.One<Boolean> isCancelled = Sinks.one();

		// Hook the cancellation into the context
		request.configureExecutionInput((executionInput, builder) -> builder
				.graphQLContext(Map.of(IS_CANCELLED_CONTEXT_KEY, isCancelled.asMono()))
				.build());

		// Execute the call
		return chain.next(request)
				// Set the timeout
				.timeout(REQUEST_TIMEOUT,
						// Send the cancellation signal
						Mono.fromRunnable(() -> isCancelled.tryEmitValue(true))
								// Throw the timeout error
								.then(Mono.error(new ResponseStatusException(HttpStatus.REQUEST_TIMEOUT))));
	}
}

// We wrap each controller method with a function to apply that state via the `takeUntilOther` mechanism
public class ControllerUtils {
	public static <T> Mono<T> wrapResponse(
			Mono<Void> isCancelled,
			Mono<T> source) {
		// Return the input source
		return source
				// Add the cancellation hook
				.takeUntilOther(isCancelled);
	}
	
	public static <T> Flux<T> wrapResponse(
			Mono<Void> isCancelled,
			Flux<T> source) {
		// Return the input source
		return source
				// Add the cancellation hook
				.takeUntilOther(isCancelled);
	}
}

[rstoyanchev]

Thanks for all the feedback. We can do something along the lines of #450 (comment), and propagate the cancellation from ContextDataFetcherDecorator, which already deals with similar concerns.

This would have an important limitation though since we can only propagate cancellation via reactive types. There is no way to cancel a controller method executing synchronously. For that at best, we could have a flag in the GraphQLContext the controller method, and the controller method would have to check it periodically. I don't see any other option.

[bclozel]

I've just addressed the CANCEL signal propagation part in #1149 - repurposing this issue to be about the WebGraphQlInterceptor only.

[bclozel]

This is now available in 1.4.0-SNAPSHOT with a new section in the documentation.

[bclozel]

Reopening to reconsider the blocking case.

[tstocker-black-cape]

I have a sort of "hacky" workaround in my app to power the cancellation of blocking tasks. It could certainly use some polish, but it works well enough. I'll share it in case it inspires a more general solution.

protected <T> T wrapResponse(Callable<T> task) { // NOSONAR
		// Submit the work to the virtual thread executor
		Future<T> future = VIRTUAL_EXECUTOR.submit(task);

		// Catch any exceptions
		try {
			// Wait for the task to complete or time out
			return future.get(REQUEST_TIMEOUT_SECONDS, SECONDS);
		} catch (Exception e) {
			// Cancel the task
			future.cancel(true);

			// If this is an interrupted exception, interrupt the current thread
			if (e instanceof InterruptedException) {
				Thread.currentThread().interrupt();
			}

			// If this is an execution exception, extract the cause
			Throwable cause = e;
			if (e instanceof ExecutionException executionException) {
				cause = executionException.getCause();
			}

			// Throw the wrapped exception
			if (cause instanceof RuntimeException runtimeException) {
				throw runtimeException;
			} else {
				// TODO make a better exception...
				throw new RuntimeException(cause);
			}
		}
	}

This wrap function is then used in the controller methods like this

@QueryMapping
public Person person(UUID id) {
  return wrapResponse(()->personService.findById(id));
}

Obviously for a general solution you wouldn't want to have an explicit wrapper function, but I didn't want to get way down in the weeds with the GraphQL library, so this worked for my app.

I should also note that my app is using virtual threads as configured by sprint boot. So the future.get() is being called from a virtual thread. This would not be a great solution if one were to use platform threads to call it.

[bclozel]

Re-closing this issue in favor of #1153