|Summary:||IllegalStateException using CompressionFilter with Tomcat 7.0.21/22|
|Product:||Tomcat 7||Reporter:||Kari Scott <kari.scott>|
|Component:||Jasper||Assignee:||Tomcat Developers Mailing List <dev>|
|Attachments:||Simple test to recreate CompressionFilter causing IllegalStateException|
Description Kari Scott 2011-10-04 15:36:57 UTC
Created attachment 27685 [details] Simple test to recreate CompressionFilter causing IllegalStateException Using Tomcat 7.0.21 or 7.0.22 with jdk1.6.0_26 on Solaris 10, mod_ajp and Apache 2.2.21, we get the following logged exception when a sendRedirect is used in a jsp and the CompressionFilter is enabled. It does not occur using 7.0.20. I've attached a gzipped tar file containing a simple test that reproduces the problem. test.jsp just contains a sendRedirect to test2.jsp and I've included the source code for the CompressionFilter and a simple web.xml file with this filter's entry. Hitting test.jsp logs the exception below. SEVERE: Servlet.service() for servlet [jsp] in context with path  threw exception [java.lang.IllegalStateException: getWriter() has already been called for this response] with root cause java.lang.IllegalStateException: getWriter() has already been called for this response at org.apache.catalina.connector.Response.getOutputStream(Response.java:594) at org.apache.catalina.connector.ResponseFacade.getOutputStream(ResponseFacade.java:199) at com.tirerack.filters.CompressionResponseStream.<init>(CompressionResponseStream.java:47) at com.tirerack.filters.CompressionServletResponseWrapper.createOutputStream(CompressionServletResponseWrapper.java:172) at com.tirerack.filters.CompressionServletResponseWrapper.getWriter(CompressionServletResponseWrapper.java:250) at org.apache.jasper.runtime.JspWriterImpl.initOut(JspWriterImpl.java:125) at org.apache.jasper.runtime.JspWriterImpl.flushBuffer(JspWriterImpl.java:118) at org.apache.jasper.runtime.PageContextImpl.release(PageContextImpl.java:190) at org.apache.jasper.runtime.JspFactoryImpl.internalReleasePageContext(JspFactoryImpl.java:123) at org.apache.jasper.runtime.JspFactoryImpl.releasePageContext(JspFactoryImpl.java:80) at org.apache.jsp.test_jsp._jspService(test_jsp.java:74) at org.apache.jasper.runtime.HttpJspBase.service(HttpJspBase.java:70) at javax.servlet.http.HttpServlet.service(HttpServlet.java:722) at org.apache.jasper.servlet.JspServletWrapper.service(JspServletWrapper.java:433) at org.apache.jasper.servlet.JspServlet.serviceJspFile(JspServlet.java:389) at org.apache.jasper.servlet.JspServlet.service(JspServlet.java:333) at javax.servlet.http.HttpServlet.service(HttpServlet.java:722) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:304) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:210) at com.tirerack.filters.CompressionFilter.doFilter(CompressionFilter.java:194) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:243) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:210) at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:224) at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:169) at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:472) at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:168) at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:100) at org.apache.catalina.valves.AccessLogValve.invoke(AccessLogValve.java:929) at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:118) at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:405) at org.apache.coyote.ajp.AjpProcessor.process(AjpProcessor.java:200) at org.apache.coyote.AbstractProtocol$AbstractConnectionHandler.process(AbstractProtocol.java:515) at org.apache.tomcat.util.net.JIoEndpoint$SocketProcessor.run(JIoEndpoint.java:302) at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908) at java.lang.Thread.run(Thread.java:662)
Comment 1 Jess Holle 2011-10-04 15:57:44 UTC
I find this a bit funny as I had exactly the same issue with my own CompressionFilter. The issue is that 7.0.21 produces a response body *inside* sendRedirect(). This entails it obtaining a response writer therein, which your filter can't know has occurred as it's doing this at a level of the filter chain your filter can't see. This issue seemed easy enough to address -- for the test cases I initially saw. Unfortunately for your test case things are not so simple. I'm not at all sure how one can do a CompressionFilter with this Tomcat change in place -- as filters can no longer tell when it is safe to call getOutputStream(). I'm half inclined to remove the Tomcat "fix" that caused this from my Tomcat binary.
Comment 2 Mark Thomas 2011-10-05 02:41:26 UTC
How about a new configuration attribute on the context that allows this behaviour to be enabled / disabled per context? See https://issues.apache.org/bugzilla/show_bug.cgi?id=41718 for why this behaviour was introduced.
Comment 3 Jess Holle 2011-10-05 04:13:54 UTC
Overall the RFC cited and the existing servlet API do not combine well when one considers things like compression Filters. I ended up figuring out a way to work around this in my own compression filter (with targeted catching of IllegalStateExceptions and appropriate fallback actions), but I will have to say that it wasn't pretty.
Comment 4 Kari Scott 2011-10-05 13:57:57 UTC
If I had to choose between a new context attribute or mucking with my CompressionFilter with something "that isn't pretty", well, I'm going to go with the attribute. :-) Was the original issue of not including the hypertext note/link in the response body for sendRedirects actually causing trouble or was it just added to conform to the RFC?
Comment 5 Christopher Schultz 2011-10-05 14:33:50 UTC
FWIW, the javadoc for javax.servlet.Filter specifically mentions "compression" as a type of Filter one might want to build. Though not exactly part of the spec, Tomcat should try it's best not to get in the way of building such Filters. Perhaps one way would be to re-write the patch for r1156533 to use OutputStream instead of Writer. Something I'm not getting is this: Why is your code (say, org.apache.jsp.test_jsp._jspService(test_jsp.java:74)) running after a call to sendRedirect? Also, if *your* code calls sendRedirect(), why isn't the request *already* wrapped in your compression wrapper, so that calling sendRedirect which calls getWriter ends up calling your wrapped-getWriter method?
Comment 6 Jess Holle 2011-10-05 14:41:46 UTC
If you look at the JSP source in question, it simply calls sendRedirect() in a scriptlet. The issue here is that the auto-generated JSP source, whitespace, etc, will try to write more stuff after that call. It would probably be more correct to do: response.sendRedirect(...); return; in the scriptlet -- which may avoid the issue. In any case this sort of attention to detail wasn't necessary prior to this change, though, so there are bound to be existing JSP pages, servlets, etc, where this sort of sloppiness exists. Thus I believe we should endeavor to be forgiving to such cases. Also using an output stream rather than a writer won't solve the issue as I believe the JSP page in question would then produce an exception *without* a compression filter in place.
Comment 7 Jess Holle 2011-10-05 14:46:20 UTC
I'd think the ideal case might be to hide the sendRedirect() response body generation from getOutputStream() and getWriter() entirely. getOutputStream() and getWriter() could really return no-op output streams and writers after sendRedirect() has been called [as the servlet spec says nothing should be written to the response after this point] and throw IllegalStateExceptions only when things *outside* sendRedirect() have tried to obtain both an output stream and a writer.
Comment 8 Kari Scott 2011-10-05 14:51:20 UTC
It's exactly what Jess said, it's the auto-generated JSP code that's the problem. And, we do have returns after all of our sendRedirects but in this simple test it wouldn't compile because it's unreachable so I removed it since it didn't seem pertinent to the test. The IllegalException occurs both with and without it.
Comment 9 Jess Holle 2011-10-05 16:39:59 UTC
If making this behavior transparent to callers is seen as too much overhead/mess/whatever, then I'd vote to revert the sendRedirect() RFC fix. I actually prefer that sendRedirect() does not generate a body as I'm a fan of waste-not-want-not when it comes to IO, CPU, etc, usage. I don't know of any use case I care about that would make use of the response body of a redirect response, so including such a response body seems like a waste.
Comment 10 Konstantin Kolinko 2011-10-05 17:03:49 UTC
1. Making the behaviour optional, as suggested in Comment 2, should not hurt. 2. I think that CompressionServletResponseWrapper in this example could overwrite the sendRedirect() method and recycle its writer and outputstream. Though 1) sendRedirect() may throw an IllegalStateException, 2) o.a.c.connector.Response#sendRedirect() silently ignores the call if it is performed from within an included call. 3. I wonder whether it is possible to optimize PageContextImpl.release() so that it does not flush the buffer if response has been suspended.
Comment 11 Christopher Schultz 2011-10-05 21:36:31 UTC
(In reply to comment #6) > If you look at the JSP source in question, it simply calls sendRedirect() in a > scriptlet. The issue here is that the auto-generated JSP source, whitespace, > etc, will try to write more stuff after that call. > > It would probably be more correct to do: > response.sendRedirect(...); > return; > in the scriptlet -- which may avoid the issue. > > In any case this sort of attention to detail wasn't necessary prior to this > change, though, so there are bound to be existing JSP pages, servlets, etc, > where this sort of sloppiness exists. Thus I believe we should endeavor to be > forgiving to such cases. The same could be true for a JSP that calls sendRedirect and then continues to produce (lots of?) output. It seems reasonable for Tomcat to allow webapps to provide their own "redirect" response bodies, too. I'm starting to lean towards Mark's suggestion of having this be an application-wide setting (though being able to enable/disable it on a per-request basis might also be nice). > Also using an output stream rather than a writer won't solve the issue as I > believe the JSP page in question would then produce an exception *without* a > compression filter in place. I think I wasn't thinking clearly when I asked my original question about the wrapper being in place: the root cause is that the compression filter hasn't called response.getOutputStream before delegating the call down the filter chain. When sendRedirect() is called, getWriter is called which (IMHO) cause the opposite exception to be thrown: IllegalStateException: getOutputStream has already been called. There are places in (Tomcat's?) code where IllegalStateException is caught when calling getWriter (or getOutputStream) and then the code switches-over to use the other strategy to accommodate the current situation. Maybe Tomcat's sendRedirect code should be augmented to do such error recovery. Finally, I just have to say it: JSP can be abused in /so many/ ways, and performing a redirect from a JSP is one such example.
Comment 12 Mark Thomas 2011-10-07 22:09:05 UTC
This is now optional (sendRedirectBody on Context) and disabled by default. The change has been made to trunk and 7.0.x and will be included in 7.0.23 onwards.