Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

req.getRequestDispatcher("/test.htm").include(req, resp); does not go anywhere on jetty12ee10 on exploded deploy, works on ee9 and older jettys. #10142

Closed
andresluuk opened this issue Jul 25, 2023 · 10 comments · Fixed by #10154
Labels
Bug For general bugs on Jetty side

Comments

@andresluuk
Copy link

andresluuk commented Jul 25, 2023

Jetty version(s)
jetty12 built from source
Java version/vendor (use: java -version)
JDK 17
OS type/version
Windows/Unix
Description
Maybe the same as #10139 ?
How to reproduce?
I have a simple servlet that does forwarding into a local htm file:

	protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException {
		System.out.println("MyServlet.doGet() called");
		try {
		  req.getRequestDispatcher("/test.htm").include(req, resp);
		} catch (IOException e) {
			throw new ServletException(e);
		}
	}

test.htm is in the root of the web app.
If I deploy the application as packaged application on jetty12ee10 then it works and test.htm is shown. But If I deploy it as exploded application then I will get an empty page. The same application for jett12ee9 it works in both modes.

@andresluuk andresluuk added the Bug For general bugs on Jetty side label Jul 25, 2023
@joakime
Copy link
Contributor

joakime commented Jul 25, 2023

How is this servlet mapped?
Can you detail your <servlet> and <servlet-mapping> entries for the above Servlet?
And then what your request is, along with any context-path information for the containing WebAppContext or ServletContextHandler?

@andresluuk
Copy link
Author

andresluuk commented Jul 25, 2023

The request url is: http://localhost:8080/exp-servletContext-requestDispatcherInclude/myservlet/xxx?3731317427365001949

web.xml is:

<web-app xmlns="http://java.sun.com/xml/ns/j2ee"
	xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
	xsi:schemaLocation="http://java.sun.com/xml/ns/j2ee 
	      http://java.sun.com/xml/ns/j2ee/web-app_2_4.xsd"
	version="2.4">

  <servlet>
    <servlet-name>myServlet</servlet-name>
    <servlet-class>test.MyServlet</servlet-class>
  </servlet>
  
  <servlet-mapping>
    <servlet-name>myServlet</servlet-name>
    <url-pattern>/myservlet/*</url-pattern>
  </servlet-mapping>

  <welcome-file-list>
    <welcome-file>/myservlet</welcome-file>
  </welcome-file-list>
</web-app>

I can send the entire test application if needed, but there is not much more there.

@andresluuk
Copy link
Author

One more note here. It actually fails with ee10 in both modes on plain jetty run, but passes with ee9 in both modes. (It passed on my test before because of jrebel).

http://localhost:8080/pkg-servletContext-requestDispatcherInclude/myservlet/xxx?59122691717

pkg-servletContext-requestDispatcherInclude.zip

joakime added a commit that referenced this issue Jul 25, 2023
….jetty.servlet.Default.` prefix for ee10

+ Fix named servlet lookup NPE in ee10 ServletHandler.getMappedServlet
@joakime
Copy link
Contributor

joakime commented Jul 25, 2023

I have a testcase in PR #10149 that uses your setup (in comment #10142 (comment)) and passes (but needs the fixes in that PR to work)

@andresluuk
Copy link
Author

Sad news here. I used they jetty built from the PR#10149 but this case still fails.
One difference between the 2 cases is, that this one does not have the context-param. (I added it for testing now, but it did not change anything). I think the welcome-file part is not relevant to this case so the nullpointer fix was for this case?

@gregw
Copy link
Contributor

gregw commented Jul 26, 2023

I've reproduced the problem in branch: https://github.com/eclipse/jetty.project/tree/fix/12.0.x/10142-include-static, based off the PR #10149 (but I don't think it is related).
@lorban do you want to look at fixing it, as I think it is probably related to the simplification I suggested to you

gregw added a commit that referenced this issue Jul 26, 2023
Tests to replicate the problem in #10142
joakime added a commit that referenced this issue Jul 26, 2023
….jetty.servlet.Default.` prefix for ee10 (#10149)

+ Issue #10141 and #10142 - Reintroduce context init-param `org.eclipse.jetty.servlet.Default.` prefix for ee10
+ Fix named servlet lookup NPE in ee10 ServletHandler.getMappedServlet
+ Adding requested javadoc
lorban added a commit that referenced this issue Jul 26, 2023
joakime pushed a commit that referenced this issue Jul 26, 2023
Tests to replicate the problem in #10142
joakime pushed a commit that referenced this issue Jul 26, 2023
@joakime
Copy link
Contributor

joakime commented Jul 26, 2023

PR #10154 opened for this issue

@joakime joakime linked a pull request Jul 26, 2023 that will close this issue
lorban added a commit that referenced this issue Jul 26, 2023
Signed-off-by: Ludovic Orban <[email protected]>
lorban added a commit that referenced this issue Jul 26, 2023
Signed-off-by: Ludovic Orban <[email protected]>
@lorban
Copy link
Contributor

lorban commented Jul 26, 2023

@andresluuk Would it be possible for you to test that the changes in PR #10140 do solve this problem?

lorban added a commit that referenced this issue Jul 26, 2023
#10142 Added extra DefaultServlet include tests

Signed-off-by: Ludovic Orban <[email protected]>
Co-authored-by: gregw <[email protected]>
@andresluuk
Copy link
Author

I think the PR got merged? So I tryed out 12.0.x and my test passed on there. Thanks!

@lorban
Copy link
Contributor

lorban commented Jul 27, 2023

@andresluuk Yes, #10140 got merged shortly after I sent you that message.

I'm happy that this change made your test pass, thanks for your feedback!

@lorban lorban closed this as completed Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants