EagerContentHandler. #9051 #12077

gregw · 2024-07-23T06:56:29Z

Fix #9051 with EagerContentHandler to replace DelayedHandler

The make the EagerContentHandler.RetainedContentLoader work efficiently this PR adjusted the buffering strategy of h1, h2 and h3 to keep and reuse a retained buffer until it is mostly full.

Also fixed several bugs in XmlConfiguration:

A Set could not be used with a builder style method (kind of missing feature more than a bug)
If a property element was used in a Set it could only be a string and the type element was ignored.
When trying to find a native match, the vClass local variable was overwritten with the native TYPE being tried. This affected subsequent match attempts using the wrong vClass

gregw · 2024-07-23T06:58:21Z

@sbordet @lorban Your thoughts on doing this? If you think it is a good idea, can you contribute the h2 and h3 delayed dispatch handling to this branch.
I will then change the DelayedHandler to only be for delaying to read the entire content (buffering to disk or parsing multi-part or form content).

lorban · 2024-07-23T09:49:21Z

Implementation-wise, this looks alright assuming the same logic can be replicated in H2/H3 (but I doubt this is going to be troublesome).

But could you summarize what benefits/drawbacks this has over the existing DelayedHandler both for clarity and posterity?

gregw · 2024-07-23T14:50:54Z

@lorban wrote:

could you summarize what benefits/drawbacks this has over the existing DelayedHandler both for clarity and posterity?

The DelayedHandler is not able to truly delay the dispatch to the next handler without an execute. This is because, unlike the handle call, the demand callback is serialized with other demand/write callbacks. So if a demand callback is used to call the next handler handle method, it can dead lock if blocks waiting on a demand/write callback. Thus we need to dispatch, which kind of defeats the whole purpose of avoiding scheduling delays on the initial dispatch without content.

gregw · 2024-07-26T22:19:46Z

@sbordet @lorban Can you take this one on

gregw · 2024-08-06T07:15:24Z

I've also taken the opportunity to cleanup lots of deprecation and TODOs in HttpConnection

gregw · 2024-08-07T01:59:38Z

@lachlan-roberts Can you take over the handling of delayed multipart in this PR?

…tty-12.1.x/delayedDispatch

Signed-off-by: Lachlan Roberts <[email protected]>

…ispatch' into experiment/jetty-12.1.x/delayedDispatch

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java

gregw · 2024-08-23T01:58:38Z

@sbordet can you do the h2 handling for this
@lorban can you do the h3 handling for this

…tty-12.1.x/delayedDispatch # Conflicts: # jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java # jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/AbstractTest.java # jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletTest.java # jetty-ee11/jetty-ee11-servlet/src/test/java/org/eclipse/jetty/ee11/servlet/ServletTest.java # jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ServletTest.java

gregw · 2024-09-15T23:26:11Z

@sbordet can you do the h2 handling for this
@lorban can you do the h3 handling for this

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractConnection.java

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java

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

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java

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

jetty-ee11/jetty-ee11-servlet/src/test/java/org/eclipse/jetty/ee11/servlet/ServletTest.java

jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ServletTest.java

gregw · 2024-11-12T22:22:31Z

I found several bugs in XmlConfiguration that prevented it being used with a Builder pattern. This PR now fixes them so we can build a MultiPartConfig in XML. Adding @lachlan-roberts to reviewers so he can check that.

jetty-core/jetty-server/src/main/config/etc/jetty-eager-multipart-content.xml

jetty-core/jetty-server/src/main/java/module-info.java

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

jetty-core/jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlConfiguration.java

…e used for setters with properties

sbordet

I like this approach much better.

Mostly nits, but few more important ones.

Documentation and DistributionTests are missing, but I can add them.

jetty-core/jetty-server/src/main/config/etc/jetty-eager-content.xml

jetty-core/jetty-server/src/main/config/etc/jetty-eager-multipart-content.xml

jetty-core/jetty-server/src/main/config/modules/eager-content.mod

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/EagerContentHandler.java

jetty-core/jetty-server/src/main/config/modules/eager-content.mod

Signed-off-by: Simone Bordet <[email protected]>

The javadoc of the Servlet API says: > If the parameter data was sent in the request body, such as occurs with an HTTP POST request, then reading the body directly via getInputStream or getReader can interfere with the execution of this method. So there is no requirement to detect or ignore somebody reading before calling a parameter method

…tty-12.1.x/delayedDispatch

sbordet · 2024-11-14T09:54:22Z

@gregw don't merge yet, I'm writing the documentation.

Signed-off-by: Simone Bordet <[email protected]>

…patch'. Signed-off-by: Simone Bordet <[email protected]>

gregw · 2024-11-14T21:03:04Z

@lorban @lachlan-roberts please re-review

Signed-off-by: Simone Bordet <[email protected]>

lachlan-roberts · 2024-11-15T02:28:57Z

jetty-core/jetty-server/src/main/config/modules/eager-content.mod

+# jetty.eager.multipart.location=/tmp
+
+## The maximum number of parts that can be parsed from the MultiPart content, or -1 for unlimited.
+# jetty.eager.multipart.maxParts=100


Still a bit weird that you have -1 for default above and -1 for unlimited for these ones.

I do prefer -1 being unlimited not default, you said you can just set MAX_VALUE for unlimited, but that is a bit more annoying because you don't know from here if its an int or a long.

…tty-12.1.x/delayedDispatch

Revert delayed dispatch handling to the connections.

5d1a9f6

gregw requested review from lorban, sbordet and lachlan-roberts July 23, 2024 06:56

Revert delayed dispatch handling to the connections.

8c07278

gregw assigned sbordet and lorban Jul 26, 2024

Cleanup of HttpConnection

548770c

Delay until MultiPartFormData

ec0d325

gregw assigned lachlan-roberts Aug 7, 2024

gregw and others added 6 commits August 7, 2024 12:38

Merge remote-tracking branch 'origin/jetty-12.1.x' into experiment/je…

fb85057

…tty-12.1.x/delayedDispatch

Wait for release

da5f09f

fixed removal of deprecated methods

5993364

fixed no mimeType

3ef3d6e

PR #12077 - add test for DelayedHandler with multipart

5ede88a

Signed-off-by: Lachlan Roberts <[email protected]>

Merge remote-tracking branch 'origin/experiment/jetty-12.1.x/delayedD…

e8ab483

…ispatch' into experiment/jetty-12.1.x/delayedDispatch

lachlan-roberts previously requested changes Aug 8, 2024

View reviewed changes

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java Outdated Show resolved Hide resolved

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java Outdated Show resolved Hide resolved

gregw added 2 commits August 22, 2024 16:24

updates from review

9e1f431

fixed test

e0f5b7e

gregw requested a review from lachlan-roberts August 23, 2024 01:57

sbordet previously requested changes Sep 17, 2024

View reviewed changes

gregw requested review from sbordet, joakime and lorban November 12, 2024 22:22

lachlan-roberts requested changes Nov 13, 2024

View reviewed changes

Fixed several XmlConfiguration issues so that a Builder pattern can b…

2b0fe1b

…e used for setters with properties

gregw requested a review from lachlan-roberts November 13, 2024 07:20

sbordet reviewed Nov 13, 2024

View reviewed changes

Updates from review

e391c0b

gregw requested a review from sbordet November 13, 2024 11:07

sbordet requested changes Nov 13, 2024

View reviewed changes

jetty-core/jetty-server/src/main/config/modules/eager-content.mod Outdated Show resolved Hide resolved

jetty-core/jetty-server/src/main/config/modules/eager-content.mod Outdated Show resolved Hide resolved

Updates from review

da36c86

gregw requested a review from sbordet November 13, 2024 20:19

sbordet and others added 5 commits November 13, 2024 21:30

Added distribution test cases for EagerContentHandler modules.

6bfc885

Signed-off-by: Simone Bordet <[email protected]>

Merged eager modules to one module

4997eed

Merge remote-tracking branch 'origin/jetty-12.1.x' into experiment/je…

494fe3a

…tty-12.1.x/delayedDispatch

Fixed bad content type test

e8ce082

sbordet approved these changes Nov 14, 2024

View reviewed changes

sbordet added 2 commits November 14, 2024 15:33

Added documentation about EagerContentHandler.

fbd09fb

Signed-off-by: Simone Bordet <[email protected]>

Merged branch 'jetty-12.1.x' into 'experiment/jetty-12.1.x/delayedDis…

6fc59b4

…patch'. Signed-off-by: Simone Bordet <[email protected]>

sbordet added 2 commits November 14, 2024 22:34

Renamed properties "jetty.eager.retained" to "jetty.eager.content".

8dd0dfe

Signed-off-by: Simone Bordet <[email protected]>

Clarified documentation about EagerContentHandler.

ef2aae0

Signed-off-by: Simone Bordet <[email protected]>

lachlan-roberts approved these changes Nov 15, 2024

View reviewed changes

Merge remote-tracking branch 'origin/jetty-12.1.x' into experiment/je…

187d57b

…tty-12.1.x/delayedDispatch

lorban approved these changes Nov 15, 2024

View reviewed changes

gregw merged commit ceca92c into jetty-12.1.x Nov 17, 2024
10 checks passed

gregw deleted the experiment/jetty-12.1.x/delayedDispatch branch November 17, 2024 20:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EagerContentHandler. #9051 #12077

EagerContentHandler. #9051 #12077

gregw commented Jul 23, 2024 •

edited

Loading

gregw commented Jul 23, 2024

lorban commented Jul 23, 2024

gregw commented Jul 23, 2024 •

edited

Loading

gregw commented Jul 26, 2024

gregw commented Aug 6, 2024

gregw commented Aug 7, 2024

gregw commented Aug 23, 2024

gregw commented Sep 15, 2024

gregw commented Nov 12, 2024

sbordet left a comment

sbordet commented Nov 14, 2024

gregw commented Nov 14, 2024

lachlan-roberts Nov 15, 2024

EagerContentHandler. #9051 #12077

EagerContentHandler. #9051 #12077

Conversation

gregw commented Jul 23, 2024 • edited Loading

gregw commented Jul 23, 2024

lorban commented Jul 23, 2024

gregw commented Jul 23, 2024 • edited Loading

gregw commented Jul 26, 2024

gregw commented Aug 6, 2024

gregw commented Aug 7, 2024

gregw commented Aug 23, 2024

gregw commented Sep 15, 2024

gregw commented Nov 12, 2024

sbordet left a comment

Choose a reason for hiding this comment

sbordet commented Nov 14, 2024

gregw commented Nov 14, 2024

lachlan-roberts Nov 15, 2024

Choose a reason for hiding this comment

gregw commented Jul 23, 2024 •

edited

Loading

gregw commented Jul 23, 2024 •

edited

Loading