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

ExceptionHandlerExceptionResolver warns when re-throwing the exception cause #23233

Closed
arlowhite opened this issue Jul 3, 2019 · 1 comment
Closed
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@arlowhite
Copy link

arlowhite commented Jul 3, 2019

If an @ExceptionHandler method matches and rethrows a cause of the root exception, it will always log this warning. Instead, the warning should only be logged if the exception differs from the one bound to the ExceptionHandler method's parameter.

Instead, this code should do something like this:

        Throwable evaluatedException = null;
        try {
            if (logger.isTraceEnabled()) {
                logger.trace("Invoking @ExceptionHandler method: " + exceptionHandlerMethod);
            }
            Throwable cause = exception.getCause();
            if (cause != null) {
                // Expose cause as provided argument as well
                evaluatedException = cause;
                exceptionHandlerMethod.invokeAndHandle(webRequest, mavContainer, exception, cause, handlerMethod);
            }
            else {
                // Otherwise, just the given exception as-is
                evaluatedException = exception;
                exceptionHandlerMethod.invokeAndHandle(webRequest, mavContainer, exception, handlerMethod);
            }
        }
        catch (Throwable invocationEx) {
            // Any other than the original exception is unintended here,
            // probably an accident (e.g. failed assertion or the like).
            if (invocationEx != evaluatedException && logger.isWarnEnabled()) {
                logger.warn("Failed to invoke @ExceptionHandler method: " + exceptionHandlerMethod, invocationEx);
            }
            // Continue with default processing of the original exception...
            return null;
        }

I'm testing on Spring 4.3.24, but the code appears to be the same.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jul 3, 2019
@rstoyanchev rstoyanchev added this to the 5.2 RC1 milestone Jul 4, 2019
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jul 4, 2019
@rstoyanchev rstoyanchev self-assigned this Jul 4, 2019
@rstoyanchev
Copy link
Contributor

Both the top level exception and the cause are made available, so we should be checking both. The exception handler method could be re-throwing either one.

@rstoyanchev rstoyanchev changed the title ExceptionHandlerExceptionResolver warns when rethrowing cause exception ExceptionHandlerExceptionResolver warns when re-throwing the exception cause Jul 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants