RequestDispatcher.forward() cannot dispatch to HTML or JSP files

[GoogleCodeExporter]

1. Put a static HTML file or JSP somewhere in your web app. For example, 
WEB-INF/protected_page.html. This bug also applies to files in the root 
dir, not just under WEB-INF.

2. Write a simple Servlet that forwards to that page, and configure that 
Servlet through a normal Guice ServletModule:

@Singleton 
public class MyServlet extends HttpServlet { 
  protected void doGet(HttpServletRequest req, 
                       HttpServletResponse res) 
            throws ServletException, IOException { 
    RequestDispatcher rd = req.getRequestDispatcher(
         "/WEB-INF/protected_page.html"); 
    rd.forward(req, res); 
  } 
} 

3. You'll see error code 404, "The requested resource (/guice_bug/WEB-
INF/protected_page.html) is not available." 

I did some additional debugging this morning in Tomcat 6.0.20 and here is 
what I've found so far. If the servlet is a normal non-Guice Servlet, the 
forward works. 

org.apache.catalina.servlets.DefaultServlet line 308 calls 
request.getServletPath(): 
// ...extract the desired path directly from the request 
String result = request.getPathInfo(); 
if (result == null) { 
  result = request.getServletPath();      // RIGHT HERE, line 308 
} 

At this point the "request" is an ApplicationHttpRequest object, and its 
servletPath is "/WEB-INF/protected_page.html". 

---------- 

Now if I go through the same path with Guice, the flow is different. 
Starting with the breakpoint at line 308 again, we go into 
request.getServletPath(). 

With Guice, however, the "request" object is a 
com.google.inject.servlet.ServletDefinition object. The getServletPath() 
method looks like this: 

      @Override 
      public String getServletPath() { 
        return computePath(); 
      } 

Which goes into the computePath() method: 

      // Memoizer pattern. 
      private String computePath() { 
        if (!pathComputed) { 
          String servletPath = super.getServletPath(); 
          path = patternMatcher.extractPath(servletPath); 
          pathComputed = true; 
          if (null == path) { 
            path = servletPath; 
          } 
        } 
        return path; 
      } 
    }; 

And here is the problem -- the pathComputed field is true, and the 
Memoized path is "/managed", which is wrong. 

If I use my debugger to change the pathComputed flag to false, the 
path is recomputed properly, and results in "/WEB-INF/protected_page.html". 

This would be the correct value, and the dispatching then works. 

So the quick summary is that 
com.google.inject.servlet.ServletDefinition is caching an incorrect 
value for "path", which results in broken RequestDispatcher.forward(...). 

I suppose an easy (maybe naive) fix would be to remove the Memoizer 
idiom from this class, but I'm not familiar enough with Guice to 
understand how this would adversely impact runtime performance. 

Original issue reported on code.google.com by burke.e...@gmail.com on 29 Dec 2009 at 6:18

[GoogleCodeExporter]

Hmm, that makes sense. Do you have a suggestion on how we can preserve the 
compiled regex and still serve 
the request path?

It's going to get seriously expensive if we allow it to be compiled every time 
=( Coz 95% of requests I imagine are 
not from request dispatcher...

Let me do some thinking. Thanks for the detailed report. I'll have this fixed 
by the ny year one way or another.

Original comment by dha...@gmail.com on 29 Dec 2009 at 11:38

• Changed state: Accepted
• Added labels: Type-Defect, Priority-High

[GoogleCodeExporter]

OK I believe I have fixed this in r1135. 

I'll be optimistic as mark as fixed, can you please reopen if you feel the 
issue was not sufficiently resolved? 
Thanks!

Original comment by dha...@gmail.com on 30 Dec 2009 at 1:06

• Changed state: Fixed

[GoogleCodeExporter]

I tested with my app and RequestDispatcher appears to work as expected, thanks 
for 
knocking this out!

Original comment by burke.e...@gmail.com on 30 Dec 2009 at 9:32

[GoogleCodeExporter]

I'm not sure what I was thinking with that last comment, because I'm testing 
today 
and this isn't really fixed. I'm attaching a test program which includes JARs I 
just 
built from Subversion. I don't have the permission to re-open this issue.

Just unzip the attachment, type "ant", and then you'll have a WAR file you can 
drop 
into Tomcat for testing. The README.txt in the ZIP file explains.

I think the problem is that line 144 of ManagedServletPipeline.java never 
executes 
when I try to forward to something like /WEB-INF/private.jsp. That's because on 
line 
123, the call to servletDefinition.shouldServe(path) returns false. Because of 
this, 
the REQUEST_DISPATCHER_REQUEST is never set.

I'm sorry for any confusion I caused when I thought the issue was fixed.

Original comment by burke.e...@gmail.com on 4 Jan 2010 at 6:30

Attachments:

• guice_bug.zip

[GoogleCodeExporter]

As a workaround, application programmers can set the "$$Guice$RDR" request 
attribute 
immediately prior to forwarding:

  // TODO: set this attribute until Guice issue 455 is fixed
  req.setAttribute("$$Guice$RDR", Boolean.TRUE);
  req.getRequestDispatcher("/WEB-INF/home.jsp").forward(req, res);

Original comment by burke.e...@gmail.com on 4 Jan 2010 at 6:53

[GoogleCodeExporter]

Hmm ouch. Let me take another look. Sorry about that. I thought the test cases 
picked it up.

Original comment by dha...@gmail.com on 4 Jan 2010 at 11:17

• Changed state: Accepted

[GoogleCodeExporter]

Check out section 8.4.2 of the Servlet 2.4 specification. This mandates that 
containers must set the "javax.servlet.forward.servlet_path" request attribute.

With this in mind, I changed ServletDefinition.java slightly. Your last patch 
introduced this code:

private boolean isPathComputed() {
  return pathComputed
    && !(null != servletRequest.getAttribute(REQUEST_DISPATCHER_REQUEST));
}

I simply changed it to the standard attribute name, and it seems to resolve my 
issue:

private boolean isPathComputed() {
  return pathComputed
    && !(null != servletRequest.getAttribute("javax.servlet.forward.servlet_path"));
}

So I think you can eliminate the custom "$$Guice$RDR" attribute and instead 
check for 
the existence of the standard attribute.

Sorry no test case, hopefully this gets you one step closer to a resolution.

Original comment by burke.e...@gmail.com on 12 Jan 2010 at 9:54

[GoogleCodeExporter]

Ohh nice catch! will fix presently.

Original comment by dha...@gmail.com on 13 Jan 2010 at 12:02

[GoogleCodeExporter]

Fixed in r1137

Original comment by dha...@gmail.com on 13 Jan 2010 at 12:15

• Changed state: Fixed

[GoogleCodeExporter]

I just tried your fix with my test app and our real apps, and it works! Thanks 
so much.

Original comment by burke.e...@gmail.com on 13 Jan 2010 at 2:10

[GoogleCodeExporter]

The fix for this isn't available in a stable release, and for whatever reason, 
the setAttribute workaround proposed in comment 5 does not work for me. Any 
other ideas on how to work around this issue? It's entirely crippling for JSP 
usage.

Original comment by cdbe...@gmail.com on 12 Jul 2010 at 3:07

[GoogleCodeExporter]

If you build from trunk it should be fine, trunk is reasonably stable atm (we 
have been using it internally).

Original comment by dha...@gmail.com on 12 Jul 2010 at 4:06

[GoogleCodeExporter]

I built from trunk, and it almost fixes the problem. The remaining difficulty 
is that servlets invoked with additional path info are mishandled. It I have a 
servlet at "/rss" and I invoke it as "/rss/news" (with /news being pathinfo), 
then try to forward to "news.jsp", it dies trying to find "rsss.jsp". 
Apparently the base part of the servlet path is being overlaid on the beginning 
of the forwarding string.

Should I open a new bug for that, or reopen this one, or what?

Original comment by cdbe...@gmail.com on 13 Jul 2010 at 5:37

[GoogleCodeExporter]

Plz open a new one and point back to this bug.
Thanks

Original comment by dha...@gmail.com on 13 Jul 2010 at 5:50

[GoogleCodeExporter]

Done, as bug 522.

Original comment by cdbe...@gmail.com on 13 Jul 2010 at 5:56