DefaultHandlerExceptionResolver handling of media type exception on Tomcat 7+

[shanman190]

Affects: 3.0.0-latest

To start out with this is referring specifically to this little piece of code found in DefaultHandlerExceptionResolver:
https://github.com/spring-projects/spring-framework/blob/master/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/support/DefaultHandlerExceptionResolver.java#L278-L287

So given this as contextual information, we can observe the test that backs this code found here:
https://github.com/spring-projects/spring-framework/blob/master/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/support/DefaultHandlerExceptionResolverTests.java#L79-L88

The behavior should be as defined in the test which I do agree completely with. However, at least when running a Tomcat embedded served Spring application this particular piece of code fails to satisfy the behavior as defined by the test. The reason behind this is that the response.sendError(HttpServletResponse.SC_UNSUPPORTED_MEDIA_TYPE) commits the response, so when the response.setHeader(String, String) method is called just a few statements later the header containing the accepted MediaTypes is thrown away.

You can see in the Tomcat code over the last few versions that this behavior has been in place since 7.x and still exists within 10.x
Tomcat 7: https://github.com/apache/tomcat/blob/7.0.108/java/org/apache/catalina/connector/ResponseFacade.java#L527-L536
Tomcat 8: https://github.com/apache/tomcat/blob/8.5.62/java/org/apache/catalina/connector/ResponseFacade.java#L467-L480
Tomcat 9: https://github.com/apache/tomcat/blob/9.0.42/java/org/apache/catalina/connector/ResponseFacade.java#L466-L479
Tomcat 10: https://github.com/apache/tomcat/blob/10.0.1/java/org/apache/catalina/connector/ResponseFacade.java#L466-L479

I believe the resolution here is that in the DefaultHandlerExceptionResolver the sendError method call should be moved down to just before the return as is the case in the similarly constructed handleHttpRequestMethodNotSupported (https://github.com/spring-projects/spring-framework/blob/master/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/support/DefaultHandlerExceptionResolver.java#L254-L263).

[shanman190]

If necessary, I can put together a sample+test that shows this issue manifesting.

[sbrannen]

@shanman190, thanks for raising the issue and providing such detail.

Your analysis and comparison to the implementation of handleHttpRequestMethodNotSupported() sound reasonable, especially when taking into account the Javadoc for javax.servlet.http.HttpServletResponse.sendError(int, String):

After using this method, the response should be considered to be committed and should not be written to.

---

If necessary, I can put together a sample+test that shows this issue manifesting.

That would be great!

But... if you are already going to put together a test, would you be willing to submit the fix along with the test as a PR?

[rstoyanchev]

I'll take care of this.

[shanman190]

@sbrannen, I definitely don't mind putting in a PR for the fix though it seems like @rstoyanchev might be handling it now. I know the Spring team prefers to have a reproducer to show the issue manifesting in practice, so that's why I offered it up alongside the detailed analysis already provided.

While the change specifically to DefaultHandlerExceptionResolver is quite simple to apply, it does illustrate that the MockHttpServletResponse doesn't quite comply with the contract defined by javax.servlet.http.HttpServletResponse at least in so far as how sendError(int) or sendError(int, String) are handled given the current tests pass while used in practice it fails. Without delving into the code and it's tests, I'm not sure how many tests across the suite would have issues with that facet of the complete change.

[sbrannen]

though it seems like @rstoyanchev might be handling it now.

Yes, Rossen will handle it.

While the change specifically to DefaultHandlerExceptionResolver is quite simple to apply, it does illustrate that the MockHttpServletResponse doesn't quite comply with the contract defined by javax.servlet.http.HttpServletResponse at least in so far as how sendError(int) or sendError(int, String) are handled given the current tests pass while used in practice it fails. Without delving into the code and it's tests, I'm not sure how many tests across the suite would have issues with that facet of the complete change.

I was also wondering if perhaps our MockHttpServletResponse should check the committed flag (like we do in resetBuffer()) in other methods like setHeader(), since that would have caused our test to fail.

Granted, the Javadoc for methods like setHeader() don't include the text "If the response has already been committed, this method throws an IllegalStateException" like we see in the Javadoc for methods like sendRedirect(). So I guess we're technically following the contract of HttpServletResponse by not checking the committed flag.

In addition, it appears that Tomcat does not throw an IllegalStateException if setHeader() is invoked after sendError(). Otherwise, you would have seen an exception in your application -- correct?

Furthermore, starting to check the committed flag now would likely break some people's tests (including our own).

So, in summary, I think we should probably leave MockHttpServletResponse as-is with regard to where we check the committed flag.

[shanman190]

@sbrannen, yes, no exception is raised. It simply just skips if the response has been committed.

My investigation was specifically around getting re-familiarized with what the default Spring Boot responses returned for the various default 4xx errors and I just so happened to be looking at the code in DefaultHandlerExceptionResolver and noticed the Accept header being set, but not showing up in the response. That resulted in me doing a little bit of digging and ultimately creating this issue to bring attention to the discrepancy.