Implement Eclipse Jetty core HTTP handler adapter#32097

gregw · 2024-01-24T06:08:57Z

This provides an implementation of a HTTP Handler Adaptor that is coded directly to the Eclipse Jetty core API, bypassing any servlet implementation.

Fixes#32035

pivotal-cla · 2024-01-24T06:09:53Z

@gregw Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

gregw · 2024-01-24T08:05:02Z

@pivotal-cla Working on getting a CCLA signed. Stand by....

.../main/java/org/springframework/web/reactive/function/server/DefaultServerRequestBuilder.java

gregw · 2024-02-01T01:42:02Z

@lachlan-roberts can you sign the individual CLA.

gregw · 2024-02-01T01:43:44Z

This PR is failing 2 tests that undertow also fails: See #25310.
Thus I'm currently suspecting bad tests or a spring bug, as both Jetty core and undertow are similar fully asynchronous integrations without servlets. We will investigate more, but any ideas that can help...

pivotal-cla · 2024-02-01T01:46:58Z

@gregw Thank you for signing the Contributor License Agreement!

...-web/src/main/java/org/springframework/http/server/reactive/JettyCoreServerHttpResponse.java

gregw · 2024-02-01T02:32:03Z

spring-web/src/main/java/org/springframework/http/server/reactive/JettyRetainedDataBuffer.java

This is the work around for the failing multipart tests (see #25310)

...g-web/src/main/java/org/springframework/http/server/reactive/JettyCoreServerHttpRequest.java

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

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

poutsma · 2024-06-24T13:09:53Z

The MultipartWebClientIntegrationTests test did not fail running from command line with jetty 12.0.11-SNAPSHOT

Great! If I understand your other comment correctly, Jetty 12.0.11 will be out by the end of June, which should give us enough time to get this PR into 6.2.0-M5.

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

lachlan-roberts · 2024-07-01T06:24:04Z

@poutsma @rstoyanchev the build of this branch is now passing all tests with the staged release of jetty-12.0.11, which should be released very soon.

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

simonbasle · 2024-07-05T10:18:38Z

@lachlan-roberts @gregw I see the artifacts appear to be on Maven Central as of June 28, but I see no announcements and the release-tracking issue jetty/jetty.project/issues/11980 is not closed. Can you confirm if we should be waiting for anything before merging?

I've retrieved Arjen's branch locally, made a couple polishes (fixing duplicate start/stop methods in JettyWebSocketClient notably), rebased on the latest main and all the tests ran successfully 👍

gregw · 2024-07-05T10:49:37Z

@simonbasle The release is out in maven central and there is no taking it back. We have some people on vacation, hence the issue process has not yet completed.

simonbasle · 2024-07-05T13:54:32Z

@lachlan-roberts unless I've missed something, you haven't yet signed the CLA have you?

lachlan-roberts · 2024-07-05T15:07:39Z

@simonbasle yes I have already signed the CLA.

This provides an implementation of an HTTP Handler Adapter that is coded directly to the Eclipse Jetty core API, bypassing any servlet implementation. This includes a Jetty implementation of the spring `WebSocketClient` interface, `JettyWebSocketClient`, using an explicit dependency to the jetty-websocket-api. Closesspring-projectsgh-32097 Co-authored-by: Lachlan Roberts <[email protected]> Co-authored-by: Arjen Poutsma <[email protected]>

See spring-projectsgh-32097

See gh-32097

See spring-projectsgh-32097

See gh-32097Closesgh-33569

JohnNiang · 2024-10-08T06:20:57Z

.../src/main/java/org/springframework/http/server/reactive/DefaultServerHttpRequestBuilder.java

 this.uri = original.getURI();
-this.headers = newHttpHeaders(original.getHeaders());
+// original headers can be immutable, so create a copy
+this.headers = newHttpHeaders(newLinkedMultiValueMap<>(original.getHeaders()));


If the headers is copied into LinkedMultiValueMap directly, it will break the case insensitive behavior of header names.
e.g.: If the headers contain an entry content-type: application/json(HTTP2 requires header names with lowercase), and the result of headers.getContentType() or headers.getFirst("Content-Type") will be null forever.

Can you raise this as a new issue?

indeed, thanks for the feedback! I've created issue #33666 to track and fix this in RC2

#### What type of PR is this? /kind bug /area core /milestone 2.20.x #### What this PR does / why we need it: This PR changes server.forward-header-strategy to native instead of framework due to a bug of Spring Framework 6.20.0-RC.1. See spring-projects/spring-framework#32097 (comment) for more. If Halo server is proxied by OpenResty which is using HTTP 2, all header names proxied into Halo server will be lowercase. This behavior makes Halo get a null header(e.g.:: `content-type: application/json`) while invoking `request.getHeaders().getContentType()`. And I found that `ServerHttpRequest` is mutated by `org.springframework.web.server.adapter.ForwardedHeaderTransformer`, so I try to use native forward-header-strategy to resolve the problem and it works very well. See [reactor.netty.http.server.DefaultHttpForwardedHeaderHandler](https://github.com/reactor/reactor-netty/blob/446683826b3020782fe3d647ef264ca7eace94f6/reactor-netty-http/src/main/java/reactor/netty/http/server/DefaultHttpForwardedHeaderHandler.java) for more. #### Does this PR introduce a user-facing change? ```release-note None ```

Prior to this commit, gh-32097 added native support for Jetty for both client and server integrations. The `JettyDataBufferFactory` was promoted as a first class citizen, extracted from a private class in the client support. To accomodate with server-side requirements, an extra `buffer.retain()` call was performed. While this is useful for server-side support, this introduced a bug in the data buffer factory, as wrapping an existing chunk means that this chunk is already retained. This commit fixes the buffer factory implementation and moved existing tests from mocks to actual pooled buffer implementations from Jetty. The extra `buffer.retain()` is now done from the server support, right before wrapping the buffer. Fixesgh-35319

gregw mentioned this pull request Jan 24, 2024
Jetty core Handler that wraps Spring HttpHandler as an adaptor webtide/spring-framework#1
Closed

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 24, 2024

gregw commented Jan 31, 2024
View reviewed changes

.../main/java/org/springframework/web/reactive/function/server/DefaultServerRequestBuilder.java Outdated Show resolvedHide resolved

gregw marked this pull request as ready for review February 1, 2024 01:52

gregw commented Feb 1, 2024
View reviewed changes

...-web/src/main/java/org/springframework/http/server/reactive/JettyCoreServerHttpResponse.java Outdated Show resolvedHide resolved

gregw commented Feb 1, 2024
View reviewed changes

...g-web/src/main/java/org/springframework/http/server/reactive/JettyCoreServerHttpRequest.java Outdated Show resolvedHide resolved

gregw commented Feb 1, 2024
View reviewed changes

...g-web/src/main/java/org/springframework/http/server/reactive/JettyCoreServerHttpRequest.java Outdated Show resolvedHide resolved

gregw commented Feb 1, 2024
View reviewed changes

...g-web/src/main/java/org/springframework/http/server/reactive/JettyCoreServerHttpRequest.java Outdated Show resolvedHide resolved

gregw mentioned this pull request Feb 1, 2024
ContentSourcePublisher throws from request jetty/jetty.project#11363
Closed

jhoeller added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Feb 5, 2024

gregwand others added 14 commits February 6, 2024 10:25

Initial boiler plate implementation of JettyCore HttpHandler Adaptor
6ff974d

More implementation of the request side
db609a7

Created JettyCoreHttpServer
7a23fbf

Created JettyCoreHttpServer
418d699

WIP on tests
930d420

WIP on tests
eb38434

Cleanup start/stop for jetty
88e7467

retainable bytebuffer!
c5a327c

Non blocking invocation type
49abe72

split out into separate files
6ca453c

minor httpHeader optimizations
ea71c61

since updated with expected release
dd72a14

Initial implementation of the writeWith methods in JettyCoreServerHtt…
50e78d3
…pResponse Signed-off-by: Lachlan Roberts <[email protected]>

Implement the ZeroCopyHttpOutputMessage interface for JettyCoreServer…
ab844d6
…HttpResponse Signed-off-by: Lachlan Roberts <[email protected]>

gregwand others added 3 commits July 1, 2024 13:48

doCommit not required
5dd2538

fix checkstyle errors
6380d3a
Signed-off-by: Lachlan Roberts <[email protected]>

fix failure for CookieIntegrationTests.partitionedAttributeTest
ca25b6c
Signed-off-by: Lachlan Roberts <[email protected]>

poutsma pushed a commit to poutsma/spring-framework that referenced this pull request Jul 3, 2024
PRspring-projects#32097- changes to address reviews for websocket
2bbd0c9
Signed-off-by: Lachlan Roberts <[email protected]>

poutsma assigned simonbasle and unassigned poutsma Jul 3, 2024

simonbasle added a commit to simonbasle/spring-framework that referenced this pull request Jul 8, 2024
Polishing
de8f7cc
See spring-projectsgh-32097

simonbasle closed this in 0a60c62Jul 8, 2024

simonbasle added a commit that referenced this pull request Jul 8, 2024
Polishing
8918a77
See gh-32097

izeye added a commit to izeye/spring-framework that referenced this pull request Sep 19, 2024
Fix Javadoc since tags in JettyCoreRequestUpgradeStrategy
9dd21bb
See spring-projectsgh-32097

izeye mentioned this pull request Sep 19, 2024
Fix Javadoc @since tags in JettyCoreRequestUpgradeStrategy#33569
Merged

sbrannen pushed a commit that referenced this pull request Sep 20, 2024
Fix Javadoc @⁠since tags in JettyCoreRequestUpgradeStrategy
159e237
See gh-32097Closesgh-33569

JohnNiang reviewed Oct 8, 2024
View reviewed changes

JohnNiang mentioned this pull request Oct 8, 2024
Fix the problem of not being able to resolve headers correctly halo-dev/halo#6786
Merged

rstoyanchev mentioned this pull request Nov 22, 2024
Remove APIs deprecated for removal in 6.2 #30608
Closed
14 tasks

bclozel mentioned this pull request Dec 11, 2024
ServerWebExchange.mutate() does not delegate modified headers back to calling instance #34067
Closed

Antolius mentioned this pull request Mar 21, 2025
Regression bug - added request header is no longer available in WebErrorHandler spring-cloud/spring-cloud-gateway#3684
Closed

bclozel mentioned this pull request Aug 27, 2025
JettyClientHttpConnector buffer leak in Spring Framework 6.2 #35319
Closed

Implement Eclipse Jetty core HTTP handler adapter#32097

Implement Eclipse Jetty core HTTP handler adapter #32097

Uh oh!

Conversation

gregw commented Jan 24, 2024

Uh oh!

pivotal-cla commented Jan 24, 2024

Uh oh!

gregw commented Jan 24, 2024

Uh oh!

Uh oh!

gregw commented Feb 1, 2024

Uh oh!

gregw commented Feb 1, 2024

Uh oh!

pivotal-cla commented Feb 1, 2024

Uh oh!

Uh oh!

gregwFeb 1, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

poutsma commented Jun 24, 2024

Uh oh!

lachlan-roberts commented Jul 1, 2024

Uh oh!

simonbasle commented Jul 5, 2024

Uh oh!

gregw commented Jul 5, 2024

Uh oh!

simonbasle commented Jul 5, 2024

Uh oh!

lachlan-roberts commented Jul 5, 2024

Uh oh!

JohnNiangOct 8, 2024

Choose a reason for hiding this comment

Uh oh!

bclozelOct 8, 2024

Choose a reason for hiding this comment

Uh oh!

simonbasleOct 8, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants