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

@ResponseBody does not work with @ExceptionHandler when reason is set in @ResponseStatus [SPR-9159] #13797

Closed
spring-projects-issues opened this issue Feb 23, 2012 · 6 comments
Assignees
Labels
in: web type: enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Feb 23, 2012

Tim Reidel opened SPR-9159 and commented

An IOException is thrown by Jetty when returning an object from an exception handler in my @Controller annotated class. Some investigation revealed that the issue was seen only when I defined an error response in the @ResponseStatus annotation.

An example @ExceptionHandler that demonstrates the issue:

@ExceptionHandler(TestException.class)
@ResponseStatus(value = HttpStatus.BAD_REQUEST, reason = "test error")
public @ResponseBody
TestObject handleTestException(TestException e)
{
	return new TestObject(e.getMessage());
}

It seems the IOException is triggered by the HTTPConnection being written to twice in ServletInvocableHandlerMethod.invokeAndHandle()

  1. in the call to setResponseStatus() in ServletInvocableHandlerMethod.invokeAndHandle()
  2. in the call to returnValueHandlers.handleReturnValue()

If there is a reason defined in the @ResponseStatus then setResponseStatus() will write output text to the HTTP response which will cause the outputStream to close. Later within returnValueHandlers.handleReturnValue() the JAXB seralization attempts to write to the outputStream which is closed. This triggers the IOException.

Below is a snippet from ServletInvocableHandlerMethod that shows the logic that generates the error page via sendError(). This is only called when this.responseReason is non-null. Thus if you define a reason in the @ResponseStatus annotation the @ResponseBody annotation won't work.

private void setResponseStatus(ServletWebRequest webRequest) throws IOException {
               if (this.responseStatus != null) {
                       if (StringUtils.hasText(this.responseReason)) {
                               webRequest.getResponse().sendError(this.responseStatus.value(), this.responseReason);
                       }
                       else {
                               webRequest.getResponse().setStatus(this.responseStatus.value());
                       }
                       // to be picked up by the RedirectView
                       webRequest.getRequest().setAttribute(View.RESPONSE_STATUS_ATTRIBUTE, this.responseStatus);
               }
       }

Affects: 3.1.1

Referenced from: commits 2295372, d52fc3b

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 24, 2012

Rossen Stoyanchev commented

Added code formatting tags.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 24, 2012

Rossen Stoyanchev commented

I'm not sure what we can do. Consider that response.setStatus(int, String) is deprecated and recommends using response.sendError(int, String) instead, which sends an HTML-formatted error page:

* Sends an error response to the client using the specified status. The
* server defaults to creating the response to look like an HTML-formatted
* server error page containing the specified message, setting the content
* type to "text/html", leaving cookies and other headers unmodified. If an
* error-page declaration has been made for the web application
* corresponding to the status code passed in, it will be served back in
* preference to the suggested msg parameter.

It sounds like what you want to do is send information in the body of the response. Obviously you can do one or the other but not both. I'm not sure what your intent is with setting the reason but it sounds like it's not what you thought it was.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 24, 2012

Tim Reidel commented

I was expecting the 'reason' string to be passed back in the "Reason-Phrase" part of the HTTP Status-Line. This should be possible even when sending back a serialized object instead of an HTML-formatted error page.

That said specifying the Reason-Phrase in this case is a "nice to have" and is not required for my (or likely any) application to work. Perhaps a viable solution is to ignore the reason code when also using the @ResponseBody annotation.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 24, 2012

Rossen Stoyanchev commented

This should be possible even when sending back a serialized object instead of an HTML-formatted error page.

In theory yes but I don't see a way to do it with the Servlet API.

bq Perhaps a viable solution is to ignore the reason code when also using the @ResponseBody annotation.

We need to consider how all other return values combine as well. In the very least we can add some clarification to the @ResponseStatus annotation. But hopefully also decide on reasonable default behavior perhaps logging a warning message as well.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 24, 2012

Rossen Stoyanchev commented

Changing from bug to improvement since there is no obvious way to do what's requested with the Servlet API.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 11, 2012

Rossen Stoyanchev commented

I've added a fix that should prevent the IOException in the original description. Essentially when @ResponseStatus has a reason and servletResponse.sendError() is called, the response is committed and should no longer be written to.
With the change, any non-null return value is ignored since it can't be written to the response anyway.

@spring-projects-issues spring-projects-issues added type: enhancement in: web labels Jan 11, 2019
@spring-projects-issues spring-projects-issues added this to the 3.1.3 milestone Jan 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web type: enhancement
Projects
None yet
Development

No branches or pull requests

2 participants