Summary: | <jsp:include> whose target performs a 'forward' does not behave as expected | ||
---|---|---|---|
Product: | Tomcat 4 | Reporter: | Pierre Delisle <pierre.delisle> |
Component: | Jasper 2 | Assignee: | Tomcat Developers Mailing List <dev> |
Status: | RESOLVED FIXED | ||
Severity: | minor | CC: | jan.luehe, mark.roth |
Priority: | P3 | ||
Version: | 4.1.24 | ||
Target Milestone: | --- | ||
Hardware: | PC | ||
OS: | All | ||
Bug Depends on: | |||
Bug Blocks: | 20940 |
The test case 2 is giving me a headache, so I won't comment on it. For the first one, I think that since you're inside an include, the forwarded page can't close the response, which I think is very logical. I think the report is invalid, sorry Pierre (and even if it's not, there are many other, way more important request dispatcher issues which have an actual impact unlike this report, waiting to be clarified by the specification, such as req/resp wrapping). I think I am against attempting to fix this bug (if confirmed it is actually one) in 4.1.x. Agree this is severity 'minor', since this is a corner case that should not occur often. > The test case 2 is giving me a headache, so I won't comment on it. With some more coffee, I know you can do it Remy :-) > For the first one, I think that since you're inside an include, the forwarded > page can't close the response, which I think is very logical. That was my impression at first, but after checking with the spec leads, it looks like the response object is to be shared between the including and included pages. Given the following in the servlet spec: - response content is sent and committed, and closed. This would mean that the output stream of the including page is closed. > I think the report is invalid, sorry Pierre (and even if it's not, there are > many other, way more important request dispatcher issues which have an actual > impact unlike this report, waiting to be clarified by the specification, > such as req/resp wrapping). > I think I am against attempting to fix this bug (if confirmed it is > actually one) in 4.1.x. Not sure it is invalid, but clarifications from the spec leads would be important. I'll copy the spec leads on this report to make sure it eventually gets addressed. As mentioned above, this is 'minor' and I understand this may not be fixed in 4.1.x, and that there are way more important issues pending. Having it logged will hopefully ensure it does not fall through the cracks... (A JSTL bug report on <c:import> is what triggered this investigation because the behavior of <c:import> in JSTL is defined in function of <jsp:include>). I believe I have a fix for testcase_1: <jsp:include> wraps the response into an instance of org.apache.jasper.runtime..ServletResponseWrapperInclude, so that the included resource may append its output to the output buffer of the including page. Therefore, in the case of "ForwardServlet", the response object passed to RequestDispatcher.forward() is an instance of ServletResponseWrapperInclude. RequestDispatcher.forward() calls resetBuffer() on the response object, which ServletResponseWrapperInclude delegates to the wrapped response. At this point, "***1***" has not been committed to the response buffer yet: it was buffered in the JspWriter associated with the including page, to which the PrintWriter associated with the ServletResponseWrapperInclude appends. This means that calling resetBuffer() on the response does not really have any effect, and therefore, "FOO" gets appended to "***1***". On the other hand, when replacing "ForwardServlet" with "forward.jsp", the code generated for "forward.jsp" calls RequestDispatcher.forward() with a response object obtained as follows: // Make sure that the response object is not the wrapper for include while (response instanceof ServletResponseWrapperInclude) { response = ((ServletResponseWrapperInclude)response).getResponse(); } As a consequence of passing the original response to the RequestDispatcher, the response consists of just "FOO", which does not get appended to "***1***". A fix is to override ServletResponseWrapperInclude.resetBuffer() as follows: public void resetBuffer() { try { writer.clearBuffer(); } catch (IOException ioe) { } } which clears the output buffer of the including page, which is what we want. However, independently of this fix, running testcase_1 throws this exception: SEVERE: Servlet.service() for servlet jsp threw exception java.io.IOException: Stream closed at org.apache.jasper.runtime.JspWriterImpl.ensureOpen(JspWriterImpl.java:233) at org.apache.jasper.runtime.JspWriterImpl.clearBuffer(JspWriterImpl.java:188) at org.apache.jsp.test1_jsp._jspService(test1_jsp.java:48) When replacing "ForwardServlet" with "forward.jsp", it throws this exception: SEVERE: Servlet.service() for servlet jsp threw exception java.lang.IllegalStateException: getOutputStream() has already been called for this response at org.apache.coyote.tomcat5.CoyoteResponse.getWriter(CoyoteResponse.java:628) at org.apache.coyote.tomcat5.CoyoteResponseFacade.getWriter(CoyoteResponseFacade.java:192) at org.apache.jasper.runtime.JspWriterImpl.initOut(JspWriterImpl.java:165) at org.apache.jasper.runtime.JspWriterImpl.flushBuffer(JspWriterImpl.java:158) at org.apache.jasper.runtime.PageContextImpl.release(PageContextImpl.java:245) at org.apache.jasper.runtime.JspFactoryImpl.internalReleasePageContext(JspFactoryImpl.java:160) at org.apache.jasper.runtime.JspFactoryImpl.releasePageContext(JspFactoryImpl.java:120) at org.apache.jsp.test11_jsp._jspService(test11_jsp.java:52) Still investigating .... Jan I find the spec's section on the RD to be impossible to understand, and, IMO, self contradicting. Wrapping is the most obvious issue. I think I'll veto any RD change until all these issues are resolved, since I'm personally tired of trying to hack something in the dark (and hopefully, the section will be reworked so that it can be implemented without too many headaches) :) Remy, I agree in general with your comments about the RD. However, I think this particular case (testcase_1) is covered by SRV.8.4: "If output data exists in the response buffer that has not been committed, the content must be cleared before the target servlet's service method is called." and "Before the forward method of the RequestDispatcher interface returns, the response content must be sent and committed, and closed by the servlet container." My proposed change (in org.apache.jasper.runtime.ServletResponseWrapperInclude) will make the "ForwardServlet" case consistent with <jsp:forward>, which already implements the required behaviour. (In reply to comment #2) > Agree this is severity 'minor', since this is a corner case that > should not occur often. ... > > I think I am against attempting to fix this bug (if confirmed it is > > actually one) in 4.1.x. This bug does not seem to be fixed yet. This functionality is needed, it is not a "corner case". Tomcat 5.0.28 seem to have this bug too. Please, fix it, including Tomcat 4.1.x too. Quick update. This is partially fixed in 5.5.x and I have just ported the same partial fix to 4.1.x. The current state is now: - for 4.1.x and 5.5.x the resulting pages display correctly in all cases - for 4.1.x and 5.5.x console messages display as expected with forward.jsp - for 4.1.x and 5.5.x no console messages are displayed with ForwardServlet - for 4.1.x and 5.5.x the IlegalStateException is never thrown in test case 2 - for 5.5.x an addtional warning is logged to the console with ForwardServlet I'll look at these remaining issues next. I have just taken a closer look at the remaining issues and the behaviour seen is as expected. The use of newlines and "***2***" etc in the test jsps was causing output to be written after the response had been committed. With all the superfluous output removed, the test cases behaved as expected. Specifically: - for 4.1.x and 5.5.x console messages are displayed with ForwardServlet - for 4.1.x and 5.5.x the IlegalStateException is thrown for test case 2 The exceptions Jan referred to in comment #3 no longer appear. The additional warnings in 5.5.x are just that. The response has been committed so trying to flush the buffer on the release of the PageContext fails. This is expected. |
It appears that tomcat does not behave properly when a JSP page invokes a <jsp:include> whose target performs a 'forward'. Details below. --- SETUP Foo.html <!-- An HTML page that simply outputs "FOO" --> FOO ForwardServlet.java: /** * A servlet that performs a RequestDispatcher.forward() * to "Foo.html". */ package tsc.servlet; import java.io.IOException; import javax.servlet.*; public class ForwardServlet extends GenericServlet { public void service(ServletRequest req, ServletResponse res) throws ServletException, IOException { ServletContext context = getServletContext(); String path="/Foo.html"; RequestDispatcher rd = context.getRequestDispatcher(path); rd.forward(req, res); } } Forward.jsp: <!-- A JSP page that performs a <jsp:forward> to "Foo.html". --> <jsp:forward page="Foo.html"/> --- TestCase 1 With the following example: TestCase1.jsp ------------- ***1*** <jsp:include page="/ForwardServlet"/> ***2*** <% System.out.println("TestCase1.jsp"); %> The expected result is that only "FOO" will be displayed on the resulting page, and "TestCase1.jsp" will be displayed on the console. Here is why: In JSP 5.4, it is stated that: "Inclusion is into the current value of out." ... "An included page only has access to the JspWriter object and it cannot set headers. This precludes invoking methods like setCookie. Attempts to invoke these methods will be ignored. The constraint is equivalent to the one imposed on the include method of the RequestDispatcher class." Although it would not hurt the spec to be a little more specific, the assumption here is that the RequestDispatcher.include() has to be called with a response object whose 'writer' is shared with the "including" page. Given that the target of the 'include' is a servlet that performs a RequestDispather.forward(), the following will happen (as per SRV 8.4): - output data in the response buffer is cleared - response content is sent and committed, and closed. > From the viewpoint of our original TestCase1.jsp page, this therefore means that: - "***1***" is added to the response output - "***1***" gets cleared from the response output - FOO is added to the response output - the response output is closed - '***2***' does not get added to the response output because it has already been closed - The 'System.out()' statement gets executed. If this example is run with Tomcat 4.1.24, the output is: ***1*** FOO and "TestCase1.jsp" is NOT displayed on the console. Not as expected. --- Moreover, with the following example: TestCase2.jsp ------------- ***1*** <jsp:include page="/ForwardServlet"/> ***2*** <% System.out.println("TestCase2-a.jsp"); %> <jsp:include page="/ForwardServlet"/> ***3*** <% System.out.println("TestCase2-b.jsp"); %> The expected result is that "TestCase2-a.jsp" will be displayed on the console and that an IllegalStateException will be thrown. [Exception should be thrown because the response has already been committed with the first forward (via the first include), and a forward cannot be done (via the second include) when a response has already been committed.] If this example is run with Tomcat 4.1.24, the output is: ***1*** FOO nothing is displayed on the console, and no exception is thrown. --- Also, in the above two test cases, if ForwardServlet is replaced by Forward.jsp as the target of the <jsp::include>'s: TestCase1 works as expected TestCase2 FOO "TestCase2-a.jsp" is displayed on the console IllegalStateException is not thrown