Bug 21440

Summary: <jsp:include> whose target performs a 'forward' does not behave as expected
Product: Tomcat 4 Reporter: Pierre Delisle <pierre.delisle>
Component: Jasper 2Assignee: 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    

Description Pierre Delisle 2003-07-09 18:14:55 UTC
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
Comment 1 Remy Maucherat 2003-07-09 18:56:42 UTC
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.
Comment 2 Pierre Delisle 2003-07-09 19:18:27 UTC
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>).
Comment 3 Jan Luehe 2003-07-09 22:40:17 UTC
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

Comment 4 Remy Maucherat 2003-07-09 22:52:02 UTC
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) :)
Comment 5 Jan Luehe 2003-07-09 23:09:00 UTC
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.
Comment 6 Michael Jouravlev 2005-04-01 20:39:29 UTC
(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.
Comment 7 Mark Thomas 2005-12-07 23:56:52 UTC
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.
Comment 8 Mark Thomas 2005-12-21 00:11:59 UTC
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.