Skip to content

Support Callable as a return value from controller methods for asynchronous execution #316

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dugenkui03 opened this issue Mar 5, 2022 · 10 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@dugenkui03
Copy link
Contributor

Asynchronous execution is key point to ensure high performance of query, details in asynchronous-execution.

It seems that all the invoketion of @controller handler method is serial by default. Will spring graphql support asynchronous execution of fetching field (probably similar to mvc-ann-async)?

@dugenkui03 dugenkui03 changed the title Asynchronous execution and fetching field in paraller Asynchronous execution and fetching field in parallel Mar 5, 2022
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 5, 2022
@rstoyanchev
Copy link
Contributor

I'm not sure I understand what you're missing. You can return Flux, Mono from controller methods.

@rstoyanchev rstoyanchev added the status: waiting-for-feedback We need additional information before we can continue label Mar 8, 2022
@dugenkui03
Copy link
Contributor Author

dugenkui03 commented Mar 9, 2022

@rstoyanchev Thanks for your response. Will spring-graphql support 'concurrent way' for the convenience of developers who prefer concurrent api.

reactor way in spring-graphql

I rewrite GreetingController#greeting to support fetching field asynchronously. If this is what your mean, (and this seems to be the only way for supporting execution asynchronously). Developers must be familiar with reactor api.

public class GreetingController {

	@QueryMapping
	public Mono<String> greeting() {
		return Mono.fromSupplier(() -> {
			RequestAttributes attributes = RequestContextHolder.getRequestAttributes();
			return "Hello " + attributes.getAttribute(RequestAttributeFilter.NAME_ATTRIBUTE, SCOPE_REQUEST);
		}).subscribeOn(Schedulers.parallel());
	}
}

concurrent way in graphql-java

Binding field with the async dataFetcher and assigning the Executor. graphql engine will fetching field in parallel.

public class AsyncDataFetcher<T> implements DataFetcher<CompletableFuture<T>> {

    /**
     * @param wrappedDataFetcher the data fetcher to run asynchronously
     * @param executor           the executor to run the asynchronous data fetcher in
     */
    public static <T> AsyncDataFetcher<T> async(DataFetcher<T> wrappedDataFetcher, Executor executor) {
        return new AsyncDataFetcher<>(wrappedDataFetcher, executor);
    }

    @Override
    public CompletableFuture<T> get(DataFetchingEnvironment environment) {
        return CompletableFuture.supplyAsync(() -> {
            //
        })
    }
}

concurrent way in spring-graphql I expected

Be similar to mvc-ann-async:

  1. Configure Executor( or AsyncTaskExecutor) in GraphQlSource.Builder;
  2. If the return value type is Callable, and the dataFetcher will be decorate by AsyncDataFetcher#async.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Mar 9, 2022
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Mar 10, 2022

Thanks for elaborating. I see now the perspective you're coming from.

A Callable is appealing but almost too simple. Once you make something asynchronous, it comes with a range of concerns. The more complete form of the Callable return value in Spring MVC is WebAsyncTask which also accepts a timeout, an Executor, along with error and completion callbacks. For all that this is a rather rudimentary mechanism that predates Reactor (as well as CompletableFuture for that matter) which provides options to compose further asynchronous stages, handle timeouts, and so on. This is before we even start talking about subscriptions and streaming. Spring MVC again has specialized types for SSE and response body streaming, again predating Reactor and Java 8.

In short, we've no plans to rebuild such purpose-built asynchronous facilities as we have in Spring MVC. These days both Spring MVC and Spring WebFlux support Reactor return values and that is the approach in Spring GraphQL from the start to make Reactor a core dependency, so you'll encounter it in all areas including client API, server side (interception, exception resolvers, etc), and testing (subscription streams)..

That said I see no reason why you couldn't reach a similar level of convenience with Reactor while also retaining the ability to compose multiple async operations together, deal with timeouts, and so on:

public class GreetingController {

	@QueryMapping
	public Mono<String> greeting() {
		RequestAttributes attributes = RequestContextHolder.getRequestAttributes();
		String name = attributes.getAttribute(RequestAttributeFilter.NAME_ATTRIBUTE, SCOPE_REQUEST);
		return executeAsync(() -> "Hello " + name);
	}

	private <T> Mono<T> executeAsync(Callable<T> callable) {
		return Mono.fromCallable(callable).subscribeOn(Schedulers.parallel());
	}

}

@pinguinjkeke
Copy link

Can we use suspend in kotlin without any other frameworks?

@dugenkui03
Copy link
Contributor Author

dugenkui03 commented Mar 10, 2022

@rstoyanchev Yes, I realize Reactor is a powerful and convenient tool. It is the first time that I spend lots of time studying Reactor, because I do encounter it in all areas of spring-graphql , as well as other spring document.

In fact, I am also familiar with graphql-java,(I used graphql-java for two years and fed back lots of code to graphql-java repository). This proposal is based on the reality of the use of graphql(java) and Reactor in China:

  1. There are few developers who use Reactor around me. Relying heavily on Reactor for asynchronous execution will prevents developers in China from using spring-graphql, but will rarely promote the use of Reactor;

  2. I always thought that It would be great that let developers focus on business logic without paying attention to Reactor/CompletableFuture( not write code about Mono/Flux/CompletableFuture). Asynchronous execution is implemented by @Configuration, @Bean and @Component.

Thanks for your elaborating. Maybe this issue should be discussed after spring-graphql has been more widely used and more feedback.

@He-Pin
Copy link

He-Pin commented Mar 10, 2022

I think the currently implemetation is pretty well, as our internal GraphQL implementation which named TQL is the same which only need the deverper retun an Async or Reactive type, the the framework itself will lift it into the same reactive type. If the user want any parallelism , they can do themself, the framework itself runs on a dedicated threadpool.
We do have some @MaxBatchSize@Maxparallelism@Blocking annotations, but that's another story.

So the question is how you manage blocking?

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Mar 17, 2022

@dugenkui03 think we could consider some simple steps to assist a data fetching controller method to become asynchronous.

For example considering that GraphQL Java expects CompletableFuture, we could turn a Supplier<?> return value from a controller method into CompletableFuture.supplyAsync(supplier, this.executor) along with some executor, configured centrally.

However, I would draw the line there. For anything more, e.g. want a timeout, or a different executor, just use CompletableFuture from the controller method. Similarly if you are integrating with some asynchronous API and need something more like the DeferredResult in Spring MVC (vs Callable or Supplier), then I would say the same, just use CompletableFuture.

What do you think?

@rstoyanchev
Copy link
Contributor

@hepin1989 if we support Supplier<?> as a return value that would be similar to the idea of an @Blocking annotation.

@pinguinjkeke you can use suspend but Reactor is still a required dependency and used in our APIs where contracts need to be asynchronous or where we need to compose asynchronous logic. This is comparable to WebFlux.

@dugenkui03
Copy link
Contributor Author

@rstoyanchev @hepin1989 Thanks for your replay.
Maybe I am really outdate, and we should use Reactor just like CompletableFuture.

We could turn a Supplier<?> return value from a controller method into CompletableFuture.supplyAsync(supplier, this.executor) along with some executor, configured centrally.

Simple but useful method, you may think that it is not elegant, such as timeout problem and thread management. But in brief, I think support Supplier(or others) is better than just Reactor. The matter is that how to support flexibly.

@pinguinjkeke
Copy link

@rstoyanchev thanks for the answer. It will be a good push for spring-graphql if we have Kotlin example which shows how it's simple to start.

@rstoyanchev rstoyanchev added this to the 1.0.0-RC1 milestone Mar 24, 2022
@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Mar 24, 2022
@rstoyanchev rstoyanchev self-assigned this Mar 24, 2022
@rstoyanchev rstoyanchev modified the milestones: 1.0.0-RC1, 1.0.0 Apr 19, 2022
rstoyanchev added a commit that referenced this issue May 16, 2022
Use GraphQLContext as input instead of ExecutionInput,
DataFetchingEnvironment and BatchLoaderEnvironment.

See gh-316
@rstoyanchev rstoyanchev changed the title Asynchronous execution and fetching field in parallel Support Callable as a return value from controller methods for asynchronous execution May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants