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

Jetty 12 inserted handler in ee10 servlet context #9927

Merged
merged 32 commits into from
Jun 22, 2023

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Jun 19, 2023

This PR refactors the ee10 handing of servlet API request and response objects:

  • The ServletContextHandler matches the request to a servlet and creates a one time only ServletContextRequest and a ServletContextResponse
  • A reusable ServletChannel object with all the heavy weight HttpInput and HttpOutput object is associated with the ServletContextRequest and ServletContextResponse.
  • Once the handling reaches the ServletHandler, the possibly wrapped request, response and callback are associated with the ServletChannel before handling.
  • Were possible the ServletApiRequest and ServletApiResponse use the possibly wrapped request/response

I have added tests to check that GzipHandler can now be nested inside of an EE10 context.

@gregw gregw requested a review from lachlan-roberts June 19, 2023 12:21
@gregw gregw marked this pull request as ready for review June 19, 2023 14:29
@gregw gregw requested review from janbartel and lorban June 19, 2023 14:34
@janbartel
Copy link
Contributor

@gregw can you edit the initial post to include a sentence or two on the motivation for the PR?

Copy link
Contributor

@janbartel janbartel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a trivial javadoc error. However, it should be fixed. Also, for a more thorough review, I think we need a hangout.

@gregw gregw changed the title Jetty 12 ee10 late servlet wrap Jetty 12 inserted handler in ee10 servlet context Jun 19, 2023
@gregw gregw requested a review from janbartel June 19, 2023 15:47
Copy link
Contributor

@lorban lorban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1st review pass

@gregw gregw requested a review from lachlan-roberts June 20, 2023 06:02
@gregw
Copy link
Contributor Author

gregw commented Jun 20, 2023

@janbartel @lorban @lachlan-roberts I've now added ServletRequestInfo and ServletResponseInfo interfaces, which are how the ServletApiRequest/ServletApiResponse communicate with the ServletContextRequest/ServletResponse.

lorban
lorban previously approved these changes Jun 20, 2023
Copy link
Contributor

@lorban lorban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM besides a few nits.

gregw added 2 commits June 20, 2023 17:59
# Conflicts:
#	jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java
#	jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/AsyncServletTest.java
@gregw
Copy link
Contributor Author

gregw commented Jun 21, 2023

@janbartel @lorban @lachlan-roberts I think the test failure is a flake. So can I get a review on this before tonight's meeting?

lorban
lorban previously approved these changes Jun 21, 2023
janbartel
janbartel previously approved these changes Jun 22, 2023
Copy link
Contributor

@janbartel janbartel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit of an oddity with the ServletRequestInfo.getServletContextRequest() method, but other than that, +1.

gregw added 2 commits June 22, 2023 11:37
removed explicit method to navigate from Request/Response Info back to core context request/response
@gregw gregw dismissed stale reviews from janbartel, lachlan-roberts, and lorban via 21da275 June 22, 2023 10:01
@gregw
Copy link
Contributor Author

gregw commented Jun 22, 2023

@janbartel @lachlan-roberts @lorban sorry, but after that last little tweak, I need a review from at least 1 of you again

@gregw gregw merged commit 0b1c28a into jetty-12.0.x Jun 22, 2023
@gregw gregw deleted the jetty-12-ee10-late-servlet-wrap branch June 22, 2023 15:04
@grgrzybek
Copy link
Contributor

@gregw I can't track the exact part of 0b1c28a, but I think it started causing NPE in some special case.

My servlet is invoked with /gateway/x?what=include&where=/:

String what = req.getParameter("what");
String where = req.getParameter("where");
switch (what) {
	case "redirect":
		resp.sendRedirect(where);
		return;
	case "forward":
		// we can't send anything when forwarding
		req.getRequestDispatcher(where).forward(req, resp);
		return;
	case "include":
		resp.getWriter().print(">>>");
		req.getRequestDispatcher(where).include(req, resp);
		resp.getWriter().print("<<<");
		return;
	default:
}

org.eclipse.jetty.ee10.servlet.ServletApiResponse#getWriter() calls:

String encoding = getServletResponseInfo().getCharacterEncoding(true);

org.eclipse.jetty.ee10.servlet.ServletContextResponse#getCharacterEncoding(boolean) calls:

if (setContentType)
    setCharacterEncoding(encoding, EncodingFrom.DEFAULT);

so org.eclipse.jetty.ee10.servlet.ServletContextResponse#_characterEncoding is set to iso-8859-1.
Because the forward is for /, default servlet is involved (because I need to test welcome files).

org.eclipse.jetty.ee10.servlet.DefaultServlet#doGet() has this fragment:

if (contextResponse != null)
{
    String characterEncoding = contextResponse.getRawCharacterEncoding();
    if (characterEncoding != null)
        content = new ForcedCharacterEncodingHttpContent(content, characterEncoding);
}

and the constructor is:

public ForcedCharacterEncodingHttpContent(HttpContent content, String characterEncoding)
{
    super(content);
    this.characterEncoding = characterEncoding;
    String mimeType = content.getContentTypeValue();
    int idx = mimeType.indexOf(";charset");
    if (idx >= 0)
        mimeType = mimeType.substring(0, idx);
    this.contentType = mimeType + ";charset=" + this.characterEncoding;
}

the content is:

content = {org.ops4j.pax.web.service.jetty.internal.web.DefaultServlet$UnknownLengthHttpContent@4857} "UnknownLengthHttpContent@c898d8f[CompressedFormatsHttpContent@1c61db46[ResourceHttpContent@471a9af8{r=file:/data/sources/github.com/ops4j/org.ops4j.pax.web-jakarta-new/pax-web-jetty/target/b1/,ct=null}]]"
 _delegate: org.eclipse.jetty.http.content.HttpContent  = {org.eclipse.jetty.http.content.PreCompressedHttpContentFactory$CompressedFormatsHttpContent@4919} "CompressedFormatsHttpContent@1c61db46[ResourceHttpContent@471a9af8{r=file:/data/sources/github.com/ops4j/org.ops4j.pax.web-jakarta-new/pax-web-jetty/target/b1/,ct=null}]"
  _delegate: org.eclipse.jetty.http.content.HttpContent  = {org.eclipse.jetty.http.content.ResourceHttpContent@4922} "ResourceHttpContent@471a9af8{r=file:/data/sources/github.com/ops4j/org.ops4j.pax.web-jakarta-new/pax-web-jetty/target/b1/,ct=null}"
   _contentType: java.lang.String  = null
   _etag: org.eclipse.jetty.http.HttpField  = {org.eclipse.jetty.http.PreEncodedHttpField@4925} "ETag: W/"///+d0tLiXs/////73HTsQ""
   _path: java.nio.file.Path  = {sun.nio.fs.UnixPath@4924} "/data/sources/github.com/ops4j/org.ops4j.pax.web-jakarta-new/pax-web-jetty/target/b1"
   _resource: org.eclipse.jetty.util.resource.Resource  = {org.eclipse.jetty.util.resource.PathResource@4749} "file:/data/sources/github.com/ops4j/org.ops4j.pax.web-jakarta-new/pax-web-jetty/target/b1/"
  compressedFormats: java.util.Set  = {java.util.HashSet@4921}  size = 0

the underlying resource is directory, so the extension is * and the resulting content type is null.

so I'm getting NullPointerException.

My test you can run (without bothering with OSGi stuff) is https://github.com/ops4j/org.ops4j.pax.web/blob/1b26a2f8ea394d696a6780ff0fa92b2613019dff/pax-web-jetty/src/test/java/org/ops4j/pax/web/service/jetty/internal/UnifiedJettyTest.java#L361

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants