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.dataBuffer = dataBufferFactory.wrap(BufferUtil.copy(chunk.getByteBuffer())); // TODO this copy avoids multipart bugs
+		this.dataBuffer = dataBufferFactory.wrap(chunk.getByteBuffer()); // TODO avoid double slice?


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 <lachlan@webtide.com>

…HttpResponse Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

Signed-off-by: Lachlan Roberts <lachlan.p.roberts@gmail.com>

gregw · 2024-06-23T23:30:31Z

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

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 <lachlan.p.roberts@gmail.com>

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 <lachlan.p.roberts@gmail.com>

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. Closes spring-projectsgh-32097 Co-authored-by: Lachlan Roberts <lachlan@webtide.com> Co-authored-by: Arjen Poutsma <arjen.poutsma@broadcom.com>

See spring-projectsgh-32097

See gh-32097

See spring-projectsgh-32097

See gh-32097 Closes gh-33569

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

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

@@ -70,7 +71,8 @@ public DefaultServerHttpRequestBuilder(ServerHttpRequest original) {
 		Assert.notNull(original, "ServerHttpRequest is required");

 		this.uri = original.getURI();
-		this.headers = new HttpHeaders(original.getHeaders());
+		// original headers can be immutable, so create a copy
+		this.headers = new HttpHeaders(new LinkedMultiValueMap<>(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 ```

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 resolved Hide 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 resolved Hide resolved

gregw commented Feb 1, 2024

View reviewed changes

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

gregw commented Feb 1, 2024

View reviewed changes

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

gregw commented Feb 1, 2024

View reviewed changes

...g-web/src/main/java/org/springframework/http/server/reactive/JettyCoreServerHttpRequest.java Outdated Show resolved Hide 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

gregw and 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 <lachlan@webtide.com>

Implement the ZeroCopyHttpOutputMessage interface for JettyCoreServer…

ab844d6

…HttpResponse Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

gregw and others added 2 commits June 12, 2024 12:07

doCommit not required

d415757

PR spring-projects#32097 - changes to address reviews for websocket

0b4b65f

Signed-off-by: Lachlan Roberts <lachlan.p.roberts@gmail.com>

gregw and others added 3 commits July 1, 2024 13:48

doCommit not required

5dd2538

fix checkstyle errors

6380d3a

Signed-off-by: Lachlan Roberts <lachlan.p.roberts@gmail.com>

fix failure for CookieIntegrationTests.partitionedAttributeTest

ca25b6c

Signed-off-by: Lachlan Roberts <lachlan.p.roberts@gmail.com>

poutsma pushed a commit to poutsma/spring-framework that referenced this pull request Jul 3, 2024

PR spring-projects#32097 - changes to address reviews for websocket

2bbd0c9

Signed-off-by: Lachlan Roberts <lachlan.p.roberts@gmail.com>

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 0a60c62 Jul 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-32097 Closes gh-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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Eclipse Jetty core HTTP handler adapter #32097

Implement Eclipse Jetty core HTTP handler adapter #32097

gregw commented Jan 24, 2024

pivotal-cla commented Jan 24, 2024

gregw commented Jan 24, 2024

gregw commented Feb 1, 2024

gregw commented Feb 1, 2024

pivotal-cla commented Feb 1, 2024

gregw Feb 1, 2024

gregw commented Jun 23, 2024

poutsma commented Jun 24, 2024

lachlan-roberts commented Jul 1, 2024

simonbasle commented Jul 5, 2024

gregw commented Jul 5, 2024

simonbasle commented Jul 5, 2024

lachlan-roberts commented Jul 5, 2024

JohnNiang Oct 8, 2024

bclozel Oct 8, 2024

simonbasle Oct 8, 2024

		// this.dataBuffer = dataBufferFactory.wrap(BufferUtil.copy(chunk.getByteBuffer())); // TODO this copy avoids multipart bugs
		this.dataBuffer = dataBufferFactory.wrap(chunk.getByteBuffer()); // TODO avoid double slice?

Implement Eclipse Jetty core HTTP handler adapter #32097

Implement Eclipse Jetty core HTTP handler adapter #32097

Conversation

gregw commented Jan 24, 2024

pivotal-cla commented Jan 24, 2024

gregw commented Jan 24, 2024

gregw commented Feb 1, 2024

gregw commented Feb 1, 2024

pivotal-cla commented Feb 1, 2024

gregw Feb 1, 2024

Choose a reason for hiding this comment

gregw commented Jun 23, 2024

poutsma commented Jun 24, 2024

lachlan-roberts commented Jul 1, 2024

simonbasle commented Jul 5, 2024

gregw commented Jul 5, 2024

simonbasle commented Jul 5, 2024

lachlan-roberts commented Jul 5, 2024

JohnNiang Oct 8, 2024

Choose a reason for hiding this comment

bclozel Oct 8, 2024

Choose a reason for hiding this comment

simonbasle Oct 8, 2024

Choose a reason for hiding this comment