Skip to content
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

Async annotation should allow for non-blocking execution with CompletableFuture [SPR-15401] #19964

Closed
spring-issuemaster opened this issue Mar 30, 2017 · 7 comments

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Mar 30, 2017

Nikolay Georgiev opened SPR-15401 and commented

When calling Async methods that have result composed of CompletableFutures Spring blocks the thread execution by calling get() method internally. This breaks the non-blocking nature of CompletableFuture and makes the use of CompletableFuture composition unusable. More details in the StackOverflow question provided.


Reference URL: https://stackoverflow.com/questions/43095014/nested-async-calls-in-spring

Issue Links:

  • #16821 Support CompletableFuture as return value in @MessageMapping handlers
  • #19823 Consistently support CompletionStage next to CompletableFuture
@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 14, 2017

Rossen Stoyanchev commented

UPDATE: I've redacted my original response...

Upon taking a closer look I think I have a better handle on it. The purpose of @Async is to turn a method that is synchronous into an asynchronous one. Since this is done through AOP interception the method is supposed to return the synchronously produced value in a "settable" Future wrapper such as AsyncResult (or CompletableFuture#completedFuture in Java 8) in order to comply with the method signature. This is described in the Javadoc.

In other words the Future.get() in AsyncExecutionInterceptor is there simply to unwrap the synchronous return value wrapped in an AsyncResult. Now in your example ServiceB uses CompletableFuture#completedFuture as expected to return a String. ServiceA however returns the asynchronous future returned from ServiceB and that's not expected. Essentially ServiceA is already asynchronous and you don't need (should not have!) the @Async annotation on it.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 14, 2017

Rossen Stoyanchev commented

One more comment, since ServiceA actually simulates some sleeping before delegating to ServiceB. I don't think @Async is meant to solve this problem. Your ServiceA has to either block or not, it can't be both. It can however delegate to two or more other @Async services and then compose the results.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 17, 2017

Nikolay Georgiev commented

The ServiceA is @Async just so when the method is called from a @Controller, the Controller will release the server's thread (say Thread-C, which might be Tomcat's request thread). Then Thread-C will be returned to the Server's pool and the server will be able to handle more new requests. Then as we do some time consuming tasks in ServiceA's method, we also want to execute ServiceB in parallel (thus we need another async call). I admit the example with the sleep is not accurate - the work in ServiceA method in real world scenario is done before and after the ServiceB method call.

Since we have the mechanism of non-blocking CompletableFuture we should be able to use it properly.
How I imagine my workflow with the Spring's @Async is:

@Controller
public class MyController {

    @Autowired
    private AsyncServiceA asyncServiceA;

    @GetMapping('/user-will-wait')
    public CompletableFuture<String> userWillWait() {
        /* Call service and return the server thread to its pool. The client will get its result when its ready. Thanks Servlet 3. */
        return asyncServiceA.a();
    }
}

@Service
public class AsyncServiceA {

    @Autowired
    private AsyncServiceB asyncServiceB;

    @ReactiveAsync /* @NonBlockingAsync */
    public CompletableFuture<String> a() {
        /* Do some stuff. */
        CompletableFuture<String> result = asyncServiceB.b()
            .thenCompose(/* Do something with the returned value.*/)
        /* Do some other asynchronous long running stuff with the result */
        return result;
    }
}

@Service
public class AsyncServiceB {

    @ReactiveAsync /* @NonBlockingAsync */
    public CompletableFuture<String> b() {
        /* Do some stuff here as well. */
        return CompletableFuture.completedFuture("Yeah, I come from another thread.");
    }
}

Why is that bad idea? I am doing this manually (as shown in SO question description) and it works for me.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 17, 2017

Nikolay Georgiev commented

Excuse my bad formatting. I couldn't find the code style block.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 17, 2017

Rossen Stoyanchev commented

It's perfectly fine for ServiceA to compose multiple async services:

@Service
public class AsyncServiceA {
 
    @Autowired
    private AsyncServiceB serviceB;

    @Autowired
    private AsyncServiceC serviceC;
 
    public CompletableFuture<String> a() {
	return serviceB.b().then(serviceC.c());
    }
}

ServiceA does not need @Async because it is async.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 19, 2017

Rossen Stoyanchev commented

Okay so bottom line is that @Async is for sync methods and those are expected to return a sync result (i.e. AsyncResult as per the Javadoc for the annotation). So to compose multiple services, either make the top-level service completely asynchronous -- e.g. by composing other asynchronous services returning CompletableFuture - and in that case there shouldn't be an @Async annotation or make it all synchronous and use @Async to make it async.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 23, 2017

Nikolay Georgiev commented

OK, I got your point now. You are completely right. Thank you for spending so much effort to help me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.