Skip to content

Commit

Permalink
Mark response errors from @ExceptionHandler as handled
Browse files Browse the repository at this point in the history
Also fix a couple of related issues:
- add AsyncRequestNotUsableException to the list of exceptions
that imply response issues.
- handle exceptions from @ExceptionHandler regardless of whether
thrown immediately or via Publisher.

Closes gh-32359
  • Loading branch information
rstoyanchev committed Mar 6, 2024
1 parent 0955f54 commit 5f601ce
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 16 deletions.
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2023 the original author or authors.
* Copyright 2002-2024 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -39,7 +39,8 @@ public class DisconnectedClientHelper {
Set.of("broken pipe", "connection reset by peer");

private static final Set<String> EXCEPTION_TYPE_NAMES =
Set.of("AbortedException", "ClientAbortException", "EOFException", "EofException");
Set.of("AbortedException", "ClientAbortException",
"EOFException", "EofException", "AsyncRequestNotUsableException");

private final Log logger;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -307,21 +307,34 @@ private Mono<HandlerResult> handleException(
exceptions.toArray(arguments); // efficient arraycopy call in ArrayList
arguments[arguments.length - 1] = handlerMethod;

return invocable.invoke(exchange, bindingContext, arguments);
return invocable.invoke(exchange, bindingContext, arguments)
.onErrorResume(invocationEx ->
handleExceptionHandlerFailure(exchange, exception, invocationEx, exceptions, invocable));
}
catch (Throwable invocationEx) {
if (!disconnectedClientHelper.checkAndLogClientDisconnectedException(invocationEx)) {
// Any other than the original exception (or a cause) is unintended here,
// probably an accident (e.g. failed assertion or the like).
if (!exceptions.contains(invocationEx) && logger.isWarnEnabled()) {
logger.warn(exchange.getLogPrefix() + "Failure in @ExceptionHandler " + invocable, invocationEx);
}
}
return handleExceptionHandlerFailure(exchange, exception, invocationEx, exceptions, invocable);
}
}
return Mono.error(exception);
}

private static Mono<HandlerResult> handleExceptionHandlerFailure(
ServerWebExchange exchange, Throwable exception, Throwable invocationEx,
ArrayList<Throwable> exceptions, InvocableHandlerMethod invocable) {

if (disconnectedClientHelper.checkAndLogClientDisconnectedException(invocationEx)) {
return Mono.empty();
}

// Any other than the original exception (or a cause) is unintended here,
// probably an accident (e.g. failed assertion or the like).
if (!exceptions.contains(invocationEx) && logger.isWarnEnabled()) {
logger.warn(exchange.getLogPrefix() + "Failure in @ExceptionHandler " + invocable, invocationEx);
}

return Mono.error(exception);
}

@Override
public Mono<HandlerResult> handleError(ServerWebExchange exchange, Throwable ex) {
return handleException(exchange, ex, null, null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,11 @@
import org.springframework.http.codec.ServerCodecConfigurer;
import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.ControllerAdvice;
import org.springframework.web.bind.annotation.ExceptionHandler;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.ResponseBody;
import org.springframework.web.context.request.async.AsyncRequestNotUsableException;
import org.springframework.web.reactive.accept.HeaderContentTypeResolver;
import org.springframework.web.reactive.handler.SimpleUrlHandlerMapping;
import org.springframework.web.reactive.resource.ResourceWebHandler;
Expand Down Expand Up @@ -195,6 +197,14 @@ void webExceptionHandler() {
assertThat(exchange.getResponse().getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR);
}

@Test
void asyncRequestNotUsableFromExceptionHandler() {
ServerWebExchange exchange = MockServerWebExchange.from(
MockServerHttpRequest.post("/request-not-usable-on-exception-handling"));

StepVerifier.create(this.dispatcherHandler.handle(exchange)).verifyComplete();
}


@Configuration
@SuppressWarnings({"unused", "WeakerAccess"})
Expand Down Expand Up @@ -249,6 +259,16 @@ public Foo unknownReturnType() {
public Publisher<String> requestBody(@RequestBody Publisher<String> body) {
return Mono.from(body).map(s -> "hello " + s);
}

@RequestMapping("/request-not-usable-on-exception-handling")
public void handle() throws Exception {
throw new IllegalAccessException();
}

@ExceptionHandler
public void handleException(IllegalAccessException ex) throws AsyncRequestNotUsableException {
throw new AsyncRequestNotUsableException("Simulated response failure");
}
}


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -432,12 +432,13 @@ protected ModelAndView doResolveHandlerMethodException(HttpServletRequest reques
exceptionHandlerMethod.invokeAndHandle(webRequest, mavContainer, arguments);
}
catch (Throwable invocationEx) {
if (!disconnectedClientHelper.checkAndLogClientDisconnectedException(invocationEx)) {
// Any other than the original exception (or a cause) is unintended here,
// probably an accident (e.g. failed assertion or the like).
if (!exceptions.contains(invocationEx) && logger.isWarnEnabled()) {
logger.warn("Failure in @ExceptionHandler " + exceptionHandlerMethod, invocationEx);
}
if (disconnectedClientHelper.checkAndLogClientDisconnectedException(invocationEx)) {
return new ModelAndView();
}
// Any other than the original exception (or a cause) is unintended here,
// probably an accident (e.g. failed assertion or the like).
if (!exceptions.contains(invocationEx) && logger.isWarnEnabled()) {
logger.warn("Failure in @ExceptionHandler " + exceptionHandlerMethod, invocationEx);
}
// Continue with default processing of the original exception...
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.util.Locale;

import jakarta.servlet.ServletException;
import jakarta.servlet.http.HttpServletResponse;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand All @@ -50,6 +51,7 @@
import org.springframework.web.bind.annotation.ResponseBody;
import org.springframework.web.bind.annotation.ResponseStatus;
import org.springframework.web.bind.annotation.RestControllerAdvice;
import org.springframework.web.context.request.async.AsyncRequestNotUsableException;
import org.springframework.web.context.support.WebApplicationObjectSupport;
import org.springframework.web.method.HandlerMethod;
import org.springframework.web.method.annotation.ModelMethodProcessor;
Expand All @@ -65,6 +67,8 @@

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.InstanceOfAssertFactories.LIST;
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.mock;

/**
* Test fixture with {@link ExceptionHandlerExceptionResolver}.
Expand Down Expand Up @@ -401,6 +405,20 @@ void resolveExceptionViaMappedHandlerPredicate() throws Exception {
assertExceptionHandledAsBody(mav, "DefaultTestExceptionResolver: IllegalStateException");
}

@Test
void resolveExceptionAsyncRequestNotUsable() throws Exception {
HttpServletResponse response = mock();
given(response.getOutputStream()).willThrow(new AsyncRequestNotUsableException("Simulated I/O failure"));

IllegalArgumentException ex = new IllegalArgumentException();
HandlerMethod handlerMethod = new HandlerMethod(new ResponseBodyController(), "handle");
this.resolver.afterPropertiesSet();
ModelAndView mav = this.resolver.resolveException(this.request, response, handlerMethod, ex);

assertThat(mav).isNotNull();
assertThat(mav.isEmpty()).isTrue();
}


private void assertMethodProcessorCount(int resolverCount, int handlerCount) {
assertThat(this.resolver.getArgumentResolvers().getResolvers()).hasSize(resolverCount);
Expand Down

0 comments on commit 5f601ce

Please sign in to comment.