Experiment/12/improve default servlet #10222

gregw · 2023-08-04T05:48:52Z

don't wrap the httpServletRequest unless necessary due to wrapping
don't wrap the httpServletResponse unless necessary due to wrapping
send content asynchronously if large and unfiltered
Remove unused boolean return from ServletChannel.handle
added TODOs where range request handling could calculate content length

+ don't wrap the httpServletRequest unless necessary due to wrapping + don't wrap the httpServletResponse unless necessary due to wrapping + send content asynchronously if large and unfiltered + Remove unused boolean return from ServletChannel.handle + added TODOs where range request handling could calculate content length

+ Call multipartlength, even though it is always -1

gregw · 2023-08-04T05:51:33Z

@lorban @sbordet @lachlan-roberts this PR improves the efficiency of the DefaultServlet.

However, it has affected some range tests that were expecting content-length to be set, but that was only because the combined parts are less that an output buffer in size. It would be better if the mutlti-part was able to calculate the length. Perhaps it needs to pre-generate the part headers? But OK for now without the content-length

Cleanup downcasting

Protect close after lasts

Prefer the wrapped servlet request/response

Do not use Wrappers for the wrapped servlet request/response

…/improve-default-servlet

+ Use static for bytes written

gregw · 2023-08-14T07:04:46Z

@lorban welcome back. Can you review this as a priority. I think it is a good cleanup of the default servlet and removes the intrinsic blocking that itches at my sole.

gregw · 2023-08-14T08:26:09Z

@lachlan-roberts @sbordet would be good to get a review on this as well.

lorban

One thing that's making me scratch my head:

Now that ServletCoreRequest|Response have been extracted as top-level classes next to ServletApiRequest|Response I wonder if their names and proximity won't add confusion in the future?

...ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/DefaultServletTest.java

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java

jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/DefaultServlet.java

gregw · 2023-08-14T12:09:50Z

One thing that's making me scratch my head:

Now that ServletCoreRequest|Response have been extracted as top-level classes next to ServletApiRequest|Response I wonder if their names and proximity won't add confusion in the future?

Any suggestions? ServletRequestRequest? Would adding the word Wrapper help?

lorban · 2023-08-14T12:22:16Z

Since they are implementations, how about very long names?
ServletCoreRequest - > ServletDelegatingCoreRequest (IS-A core Request; delegates to servlet api)
ServletApiRequest -> CoreDelegatingServletRequest (IS-A ServletRequest; delegates to core api)

gregw · 2023-08-14T12:39:18Z

@lorban

Since they are implementations, how about very long names?
ServletCoreRequest - > ServletDelegatingCoreRequest (IS-A core Request; delegates to servlet api)
ServletApiRequest -> CoreDelegatingServletRequest (IS-A ServletRequest; delegates to core api)

I don't think "Delegating" adds very much....

We could do CoreRequestServletRequest and ServletRequestCoreRequest... or CoreServletRequest and ServletCoreRequest. but I also kind of like ServletApiRequest as it says what it does and we use the "Api" name elsewhere to indicate when we are wrapping for API reasons.

updates from review

lorban

I cannot come up with better *Request class names, so the current ones probably are reasonable.

# Conflicts: # jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContextState.java # jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java

gregw · 2023-08-15T23:26:51Z

CI failure is a flaky test... giving it one more spin....

gregw · 2023-08-16T20:54:48Z

@lorban can you re review after I updated from merge conflict
@sbordet @lachlan-roberts 2nd and 3rd opinions welcome

lachlan-roberts · 2023-08-17T06:37:28Z

...etty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipResponseAndCallback.java

+            default ->
+            {
+                if (BufferUtil.isEmpty(content))
+                    callback.succeeded();


What is the case where this happens?
If last is true do we not need to do anything?

@lachlan-roberts This can happen is something does a flush when last has already been sent. This can be needed when checking there is no data buffered in a writer.

lachlan-roberts · 2023-08-17T06:54:20Z

jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java

                        {
-                            if (sendErrorOrAbort("Insufficient content written"))
+                            long written = getBytesWritten();


This getBytesWritten() implementation is going directly to the ChannelResponse, which will be the compressed content length if Gzip is used.

Is it possible that the ServletContextResponse detects the content is incomplete because it was expecting the uncompressed content length from getBytesWritten()?

@lachlan-roberts The code is comparing the actual bytes written to the actual content-length header (if any). Either they both represent compressed content or they both don't.

lachlan-roberts · 2023-08-17T07:10:59Z

...ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/DefaultServletTest.java

@@ -2183,7 +2183,7 @@ public static Stream<Arguments> rangeScenarios()
                String body = response.getContent();

                assertThat(response, containsHeaderValue("Content-Type", "multipart/byteranges"));
-                assertThat(response, containsHeaderValue("Content-Length", "" + body.length()));
+                // TODO #10307 assertThat(response, containsHeaderValue("Content-Length", String.valueOf(body.length())));


Why were these tests passing before but not now?

@lachlan-roberts Because the content was written via the servlet HttpOutput which aggregated them and they were small enough examples to fit into a single buffer. Thus the content length was known. Now it is written without the copy, but that means the length is not known when the response is committed.

See issue.

gregw added 2 commits August 3, 2023 22:25

Improve Jetty 12 DefaultServlet

34041b5

+ Call multipartlength, even though it is always -1

gregw requested review from sbordet and lorban August 4, 2023 05:48

gregw requested a review from lachlan-roberts August 4, 2023 05:51

gregw added 8 commits August 10, 2023 06:31

Merge branch 'jetty-12.0.x' into experiment/12/improve-default-servlet

dacad5c

Fix #10229 Idle Timeout

e0e12ec

Cleanup downcasting

Fix #10229 Idle Timeout

8ee428a

Cleanup downcasting

Fix #10229 Idle Timeout

8f05ea4

Protect close after lasts

Fix #10229 Idle Timeout

81b5f96

Prefer the wrapped servlet request/response

Fix #10229 Idle Timeout

b3ad5e5

Do not use Wrappers for the wrapped servlet request/response

Merge remote-tracking branch 'origin/jetty-12.0.x' into experiment/12…

a48fe0e

…/improve-default-servlet

Improve Jetty 12 DefaultServlet

ecafdbb

+ Use static for bytes written

lorban requested changes Aug 14, 2023

View reviewed changes

gregw added 2 commits August 14, 2023 22:43

Improve Jetty 12 DefaultServlet

d624348

updates from review

Improve Jetty 12 DefaultServlet

e236777

updates from review

gregw requested a review from lorban August 14, 2023 12:50

Improve Jetty 12 DefaultServlet

90f4e0b

updates from review

lorban previously approved these changes Aug 15, 2023

View reviewed changes

Merge branch 'jetty-12.0.x' into experiment/12/improve-default-servlet

7a52148

# Conflicts: # jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContextState.java # jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java

gregw dismissed lorban’s stale review via 7a52148 August 15, 2023 21:50

lachlan-roberts reviewed Aug 17, 2023

View reviewed changes

lorban approved these changes Aug 17, 2023

View reviewed changes

gregw merged commit 5aea1e4 into jetty-12.0.x Aug 17, 2023
2 checks passed

gregw deleted the experiment/12/improve-default-servlet branch August 17, 2023 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experiment/12/improve default servlet #10222

Experiment/12/improve default servlet #10222

gregw commented Aug 4, 2023

gregw commented Aug 4, 2023

gregw commented Aug 14, 2023

gregw commented Aug 14, 2023

lorban left a comment

gregw commented Aug 14, 2023

lorban commented Aug 14, 2023

gregw commented Aug 14, 2023

lorban left a comment

gregw commented Aug 15, 2023

gregw commented Aug 16, 2023

lachlan-roberts Aug 17, 2023

gregw Aug 17, 2023

lachlan-roberts Aug 17, 2023

gregw Aug 17, 2023

lachlan-roberts Aug 17, 2023

gregw Aug 17, 2023

Experiment/12/improve default servlet #10222

Experiment/12/improve default servlet #10222

Conversation

gregw commented Aug 4, 2023

gregw commented Aug 4, 2023

gregw commented Aug 14, 2023

gregw commented Aug 14, 2023

lorban left a comment

Choose a reason for hiding this comment

gregw commented Aug 14, 2023

lorban commented Aug 14, 2023

gregw commented Aug 14, 2023

lorban left a comment

Choose a reason for hiding this comment

gregw commented Aug 15, 2023

gregw commented Aug 16, 2023

lachlan-roberts Aug 17, 2023

Choose a reason for hiding this comment

gregw Aug 17, 2023

Choose a reason for hiding this comment

lachlan-roberts Aug 17, 2023

Choose a reason for hiding this comment

gregw Aug 17, 2023

Choose a reason for hiding this comment

lachlan-roberts Aug 17, 2023

Choose a reason for hiding this comment

gregw Aug 17, 2023

Choose a reason for hiding this comment