AsyncRestTemplate changes in SPR-13413 made its harder to detect Client/Server errors [SPR-14353]

[spring-projects-issues]

Oleg Mikhailenko opened SPR-14353 and commented

After migrating to 4.2.6 our response statuses were broken for our client side. It always showed 500 error status. It turned out our client's exception handler (@ControllerAdvice) caught ExecutionException even though a server responded with 4xx/5xx error statuses.
Spring's error hander for AsyncRestTemplate (DefaultResponseErrorHandler) contains code:

public void handleError(ClientHttpResponse response) throws IOException {
	HttpStatus statusCode = getHttpStatusCode(response);
	switch (statusCode.series()) {
		case CLIENT_ERROR:
			throw new HttpClientErrorException(statusCode, response.getStatusText(),
					response.getHeaders(), getResponseBody(response), getCharset(response));
		case SERVER_ERROR:
			throw new HttpServerErrorException(statusCode, response.getStatusText(),
					response.getHeaders(), getResponseBody(response), getCharset(response));
		default:
			throw new RestClientException("Unknown status code [" + statusCode + "]");
	}
}

It's OK but then (after #17992) any of these exceptions wrapped into ExecutionException in ResponseExtractorFuture (look throw new ExecutionException(ex);):

@Override
protected final T adapt(ClientHttpResponse response) throws ExecutionException {
	try {
		if (!getErrorHandler().hasError(response)) {
			logResponseStatus(this.method, this.url, response);
		}
		else {
			handleResponseError(this.method, this.url, response);
		}
		return convertResponse(response);
	}
	catch (Throwable ex) {
		throw new ExecutionException(ex);
	}
	finally {
		if (response != null) {
			response.close();
		}
	}
}

So as result it does not matter what error response sent by server HttpClientErrorException/HttpServerErrorException. It always wrapped to ExecutionException. And to fetch response status and body I have to get inner cause and cast to appropriate type. What is not so convenient.

It would be great if you could change the code of ResponseExtractorFuture to something like:

@Override
protected final T adapt(ClientHttpResponse response) throws ExecutionException {
        try {
            if (!getErrorHandler().hasError(response)) {
                logResponseStatus(this.method, this.url, response);
            }
            else {
                handleResponseError(this.method, this.url, response);
            }
            return convertResponse(response);
        }
        catch (RestClientException ex) {
            throw ex;
        }
        catch (Throwable ex) {
            throw new ExecutionException(ex);
        }
        finally {
            if (response != null) {
                response.close();
            }
        }
    }

---

Affects: 4.2.6

Issue Links:

• @ExceptionHandler should match cause as well (e.g. for exception thrown from argument formatter) [SPR-14291] #18863 @ExceptionHandler should match cause as well (e.g. for exception thrown from argument formatter)
• AsyncRestTemplate should wrap RuntimeExceptions in ExecutionException [SPR-13413] #17992 AsyncRestTemplate should wrap RuntimeExceptions in ExecutionException

1 votes, 5 watchers

[spring-projects-issues]

Juergen Hoeller commented

Have you tried 4.3 GA? With #18863, we are already introspecting any exception cause there, including ExecutionException causes. It looks like your scenario should be covered there already?

In any case, point taken that 4.2.x does not handle such wrapped exceptions in @ExceptionHandler methods yet. I'll backport a minimal variant of #18863 for that purpose, in time for our upcoming 4.2.7 release.

As for the original motivation behind #17992, this still seems sound and in conformance with the java.util.concurrent.Future javadoc which says that ExecutionException should be thrown if "the computation threw an exception"... i.e. for any exception coming out of the target operation, allowing to differentiate between async infrastructure exceptions and actual computation exceptions.

[spring-projects-issues]

Juergen Hoeller commented

This turns out to be a bit awkward to backport isolation, so I'd rather recommend going to 4.3 GA / 4.3.1 straight away where you'll have full support for exception cause matching.