Enforce Future or void return declaration for each asynchronously executed method (e.g. with class-level @Async)

[djechelon]

spring-framework/spring-aop/src/main/java/org/springframework/aop/interceptor/AsyncExecutionInterceptor.java

Lines 113 to 127 in 79d3f5c

	Callable<Object> task = () -> {
	try {
	Object result = invocation.proceed();
	if (result instanceof Future) {
	return ((Future<?>) result).get();
	}
	}
	catch (ExecutionException ex) {
	handleError(ex.getCause(), userDeclaredMethod, invocation.getArguments());
	}
	catch (Throwable ex) {
	handleError(ex, userDeclaredMethod, invocation.getArguments());
	}
	return null;
	};

I have found an odd behaviour working with @Async-annotated classes in Spring. Please note that there is a fundamental error in my code. Unfortunately, this post has to be long and detailed.

Let's say I have already made a synchronous REST API generated by Swagger generator. Following code omits all documentation-level annotations

public interface TaxonomiesApi {
   
    ResponseEntity<GenericTaxonomyItem> disableItem(Integer idTaxonomyType, String idTaxonomy, String appSource);

}

This API is easily implemented via RestTemplate, but I won't discuss the inner details.

Now, suppose I want to provide an async version to developers consuming the API. What I have done is to create another interface with some search&replace-fu 🥋🥋

@Async
public interface TaxonomiesApiAsync extends TaxonomyApi {
   
    default CompletableFuture<ResponseEntity<GenericTaxonomyItem>> disableItemAsync(Integer idTaxonomyType, String idTaxonomy, String appSource) {
        try {
            return completedFuture(this.disableItem(idTaxonomyType, idTaxonomy, appSource));
        } catch (Exception ex) {
            return failedFuture(ex);
        }
    }
}

With the search&replace, I basically created an async-ish version of every method that should be backed by Spring's @Async annotation. My original idea was that synchronous methods can be invoked as they are, but if you instantiate TaxonomiesApiAsync you also have access to the async version.

I have discovered I made a fundamental mistake by applying the @Async annotation at interface level when the class contains both sync and async methods. I found that synchronous disableItem was performed in the same @Async context. Accoding to design (correctly), Spring found the @Async annotation at interface level so every method, including inherited ones, was invoked asynchronously.

But the method always returned null. By debugging and looking at the code, I found that Spring tries to resolve the return value of the invoked method only if it's a Future. What if the returned value is a Present object?

That means that if the returned value is not a Future<ResponseEntity<GenericTaxonomyItem>> but rather just a ResponseEntity<GenericTaxonomyItem> Spring neither throws an exception nor returns that value directly.

Example of working calling code (invoking a different method)

    protected CompletableFuture<Set<TaxonomyLegalEntityDTO>> importTaxonomyLegalEntities(int userId) {
        TaxonomySearchParameters searchParameters = new TaxonomySearchParameters();
        searchParameters.setIdTaxonomyType(amlcProperties.getTaxonomies().getTaxonomyLegalEntitiesId());
        searchParameters.setLogicalState(1);
        return taxonomiesApiAsync.getAllTaxonomyItemsAsync(searchParameters)
                .thenApply(ResponseEntity::getBody)
                .thenApply(taxonomyLegalEntityMasterDbMapping::toLegalEntity) // Costruisco i DTO che voglio utilizzare
                .whenComplete(traceLoggerConsumer("Legal entity"))
                .thenApply(dtos -> taxonomyLegalEntityManager.mergeFromMasterDb(dtos, userId))
                .whenComplete((ignored, ex) -> {
                    if (ex != null)
                        log.error("Error importing legal entities: " + ex.getMessage(), ex);
                })
                .thenApply(TaxonomyMasterDbMergeDTO::getSnapshot);
    }

Example of non-working code; the result of the CompletableFuture is always null.
In this code, I decided not to use the executor embedded in the API service, but rather the executor injected in the consuming service. So I ran a sync method in an executor, expecting the same result.

    protected CompletableFuture<Set<TaxonomyLegalEntityDTO>> importTaxonomyLegalEntities(int userId) {
        TaxonomySearchParameters searchParameters = new TaxonomySearchParameters();
        searchParameters.setIdTaxonomyType(amlcProperties.getTaxonomies().getTaxonomyLegalEntitiesId());
        searchParameters.setLogicalState(1);
        return CompletableFuture.supplyAsync(() -> taxonomiesApi.getAllTaxonomyItems(searchParameters), taxonomyBatchImportServiceExecutor)
                .thenApply(ResponseEntity::getBody)
                .thenApply(taxonomyLegalEntityMasterDbMapping::toLegalEntity)
                .whenComplete(traceLoggerConsumer("Legal entity"))
                .thenApplyAsync(dtos -> taxonomyLegalEntityManager.mergeFromMasterDb(dtos, userId))
                .whenComplete((ignored, ex) -> {
                    if (ex != null)
                        log.error("Error importing legal entities: " + ex.getMessage(), ex);
                })
                .thenApply(TaxonomyMasterDbMergeDTO::getSnapshot);
    }

Since I spent one hour debugging that problem, I decided to spend more of my after-work time to document the issue here.

Proposed fix

In the code I linked, if the instanceof check fails the returned value is simply null. I don't yet understand the implications, but what about not unwrapping the value from Future if that's not a future? I mean return result

[mdeinum]

In terms of target method signatures, any parameter types are supported. However, the return type is constrained to either void or Future. In the latter case, you may declare the more specific ListenableFuture or CompletableFuture types which allow for richer interaction with the asynchronous task and for immediate composition with further processing steps.

The documentation states the limitations in the return types only void or Future. It doesn't really make sense to allow for a return of a specific type as that would make the method call synchronous again as one would need to do a Future.get which is blocking and thus renders the @Async useless.

So I the return type isn't a Future it can return null because the other allowed return value is void.

As a solution an exception would be better imho with a clear message stating that only void or Future is supported as a return type.

[rstoyanchev]

As the documentation states and as @mdeinum pointed out, the return type must Future or void, or otherwise the calling code has to block anyway, making it pointless to involve an Executor thread, and making asynchronous methods that are meant to be synchronous.

I think this can be closed, unless @jhoeller you see some opportunity to bypass methods that don't return void or Future.

[jhoeller]

I'm inclined to explicitly throw an exception for non-Future/void return type declarations whenever we attempt to execute a method asynchronously. While this may not be much of an issue with an explicit annotated method, a class-level @Async declaration is certainly harder to track when some specific method mismatches then.

[LifeIsStrange]

Noob question @jhoeller since I assume you systematically do an instanceof/reflection check, and that could incur a slowdown, wouldn't it be better to have this check only enabled on dev/debug mode et disabled on release mode?

[djechelon]

@LifeIsStrange as you can see in the code, what is done is a == check on the returnType, which is already available (already reflected). So, it's not going to add any overhead.

As for your question about the instanceof performance, I found an interesting reading and the tl;dr says

In Java 1.8 instanceof is the fastest approach, although getClass() is very close.

Nevertheless, it doesn't apply to this fix.