ContentCachingResponseWrapper should NOT sendError() before write body [SPR-13004]

[spring-projects-issues]

Victor Lyuboslavsky opened SPR-13004 and commented

The fix for #16327 does not work with Tomcat 8.0.21

public void sendError(int sc, String msg) throws IOException {
     copyBodyToResponse();
     super.sendError(sc, msg);
     this.statusCode = sc;
}

When copyBodyToResponse() is called, Tomcat considers the response committed. Then, when sendError is called, Tomcat throws

org.glassfish.jersey.server.ServerRuntime$Responder - Error while closing the output stream in order to commit response.
java.lang.IllegalStateException: Cannot call sendError() after the response has been committed
	at org.apache.catalina.connector.ResponseFacade.sendError(ResponseFacade.java:462)
	at javax.servlet.http.HttpServletResponseWrapper.sendError(HttpServletResponseWrapper.java:120)
	at javax.servlet.http.HttpServletResponseWrapper.sendError(HttpServletResponseWrapper.java:120)
	at org.springframework.web.util.ContentCachingResponseWrapper.sendError(ContentCachingResponseWrapper.java:83)

Tomcat considers the response committed because contentLength is more than 0, and it is the same as contentWritten. From org/apache/catalina/connector/Response.java:

public boolean isAppCommitted() {
    return (this.appCommitted || isCommitted() || isSuspended()
            || ((getContentLength() > 0)
                && (getContentWritten() >= getContentLength())));
}

I propose that whenever content length > 0, sendError should not be called.

My current workaround:

public class BackendContentCachingResponseWrapper extends ContentCachingResponseWrapper {

    public BackendContentCachingResponseWrapper(HttpServletResponse response) {
        super(response);
    }

    @Override
    public void sendError(int sc, String msg) throws IOException {
        try {
            super.sendError(sc, msg);
        } catch (IllegalStateException e) {
            // ignore
        }
    }

}

---

Affects: 4.1.6

Issue Links:

• ShallowEtagHeaderFilter should write body early when HttpServletResponse.sendError() is called [SPR-11705] #16327 ShallowEtagHeaderFilter should write body early when HttpServletResponse.sendError() is called

[spring-projects-issues]

Juergen Hoeller commented

#16327 isn't ideal but at least preserves the original sequence of interaction with the HttpServletResponse, just like when it isn't wrapped at all.

So I'm fixing this with a simple catch-and-swallow of the IllegalStateException, as in your workaround...

Juergen

[spring-projects-issues]

Juergen Hoeller commented

We're falling back to the silent setStatus in case of an IllegalStateException from sendError now. However, please note that it's ultimately the sequence of interaction with the HttpServletResponse that's at odds here: You should call setStatus / sendError before deciding to write a response body. Otherwise you'll still fail against Tomcat's native response implementation when not being decorated with a ContentCachingResponseWrapper...

Juergen

[spring-projects-issues]

Juergen Hoeller commented

I've just realized that there is a subtle difference with our content writing: We implicitly call setContentLength, even if the original HttpServletResponse interaction did not do so - and this is exactly what makes Tomcat fail here. I'm wondering whether we should simply skip the setContentLength call when triggering copyBodyToResponse from sendError...

Juergen

[spring-projects-issues]

Juergen Hoeller commented

We're now only calling setContentLength in case of a regularly committed response, whereas for a sendError / sendRedirect case, we just apply the response body itself... analogous to the original HttpServletResponse interaction that we received. In combination with our defensive IllegalStateException check, this should hopefully be good enough for all scenarios.

Juergen