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!

gregw · 2024-02-01T02:00:07Z

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

+			// TODO: Why does intellij warn about possible blocking call?
+			//       Because it can block and we want to be fully asynchronous.  Use AsynchronousFileChannel
+			SeekableByteChannel channel = Files.newByteChannel(file, StandardOpenOption.READ);


Files.newByteChannel is a blocking operation, but I see that undertow also use it and just live with the warning. The chance of it actually blocking for any significant time is vanishingly small and there is no chance of dead lock.

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>

Update spring Jetty websocket usage

poutsma · 2024-04-16T14:09:59Z

@gregw Hi Greg, sorry for the recent lack of feedback. I plan to take a look at this PR soon, and hopefully we can merge it in 6.2 M2 or M3.

gregw · 2024-05-23T23:33:33Z

@poutsma Would be good to get at least an initial review, as I'm sure there is some rough edges that need to be knocked off. Giving us a few comments will get us working on this and updating... plus we want to flow the work through springboot as well.

poutsma · 2024-05-24T09:21:24Z

@gregw Fair point. I had to take some unforeseen medical leave, but plan to focus on this PR this coming week.

poutsma · 2024-05-28T14:16:44Z

@gregw First off, thank you for submitting this PR, as well as your patience with regards to us handling it. The PR seems to be quite usable, and I am sure it will make it into 6.2.

Before getting into any specific details, my main question is why do JettyCoreServerHttpRequest and JettyCoreServerHttpResponse implement the ServerHttpRequest and ServerHttpResponse interfaces, and not to extend the abstract base classes? It appears that a lot of changes in the PR derive from this decision (for instance getNativeRequest is moved from the abstract base class to the interface). While it can be argued that this refactoring is desirable, it seems unrelated and makes this PR harder to review.

How should we proceed? If you prefer, I can take the PR from here, and get back you when I have questions. Or I can provide a more thorough review.

gregw · 2024-05-28T23:01:17Z

@gregw First off, thank you for submitting this PR, as well as your patience with regards to us handling it. The PR seems to be quite usable, and I am sure it will make it into 6.2.

Awesome!

Before getting into any specific details, my main question is why do JettyCoreServerHttpRequest and JettyCoreServerHttpResponse implement the ServerHttpRequest and ServerHttpResponse interfaces, and not to extend the abstract base classes? It appears that a lot of changes in the PR derive from this decision (for instance getNativeRequest is moved from the abstract base class to the interface). While it can be argued that this refactoring is desirable, it seems unrelated and makes this PR harder to review.

Very good question. I cannot recall and looking at the code now, I see no particular reason, at least not for the request.

How should we proceed? If you prefer, I can take the PR from here, and get back you when I have questions. Or I can provide a more thorough review.

I'm flexible. I think it would be good to get you closely involved as I'm sure there a spring things that I'm doing wrong and working together would be a good way to communicate knowledge. So how about this as an approach:

I'll have a go at reacting to your first feedback: i.e. try to use the abstract base classes. That will refresh my brain on this branch (it has been a while since I looked).
You can then take over it for a while, changing anything you don't like or that is wrong
I can stay involved, helping out as you require and reviewing your changes with a Jetty eye.

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

poutsma · 2024-05-29T08:13:57Z

I'm flexible. I think it would be good to get you closely involved as I'm sure there a spring things that I'm doing wrong and working together would be a good way to communicate knowledge. So how about this as an approach:

I'll have a go at reacting to your first feedback: i.e. try to use the abstract base classes. That will refresh my brain on this branch (it has been a while since I looked).

You can then take over it for a while, changing anything you don't like or that is wrong

I can stay involved, helping out as you require and reviewing your changes with a Jetty eye.

Sounds good to me. It looks like you committed step 1 (d6986ee), so I will get started with step 2. Unless there are more changes forthcoming?

gregw · 2024-05-29T12:30:57Z

@poutsma my step 1 is pretty broken. I think it need more work. stand by....

upgrade jetty

gregw · 2024-05-29T12:57:06Z

@poutsma I've backed out the abstract response changes for now, as they broke a lot of tests. So I need to understand the difference and try again. Probably tomorrow now...

…dapter # Conflicts: # framework-platform/framework-platform.gradle

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

poutsma · 2024-05-30T08:52:54Z

@poutsma I've backed out the abstract response changes for now, as they broke a lot of tests. So I need to understand the difference and try again. Probably tomorrow now...

@gregw No worries, just let me know when you want me to take a look.

gregw · 2024-05-31T00:08:28Z

@poutsma OK I think I've cracked the abstract response now as well. There is one test that fails occasionally that I need to look into more, but I think the PR is good for you to look at now. If you want to start making changes, go for it.. or direct me.

…erAdapter' into JettyCoreHttpHandlerAdapter

poutsma · 2024-05-31T11:00:08Z

@gregw I have made a first pass at reviewing the PR, and so far have made two changes:

I have reverted all unnecessary changes to ServerHttpRequest, ServerHttpRequest and subtypes, and simplified JettyCoreHttpHandlerAdapter, JettyCoreServerHttpRequest and JettyCoreServerHttpResponse, see here.
I have replaced flatMap(mapper, 1) with concatMap(mapper) in JettyCoreServerHttpResponse as these are semantically equivalent, and we use concatMap elsewhere, see here.

Regardless of whether the changes above have been applied, the main test failure I see is MultipartWebClientIntegrationTests. Next week I will take a look at why these fail. I will also take a look at the websocket changes, as I haven't done so yet.

poutsma · 2024-06-06T09:40:16Z

I have committed several other changes, see https://github.com/poutsma/spring-framework/commits/gh-32097/.

The only test that fails is the MultipartWebClientIntegrationTests. It does not fail in the IDE, making it a bit harder to reproduce. This is the server-side stack trace I get:

java.lang.IllegalArgumentException: demand pending
    at org.eclipse.jetty.server.internal.HttpChannelState$ChannelRequest.demand(HttpChannelState.java:955) ~[jetty-server-12.0.10.jar:12.0.10]
    at org.eclipse.jetty.io.content.ContentSourcePublisher$SubscriptionImpl.process(ContentSourcePublisher.java:124) ~[jetty-io-12.0.10.jar:12.0.10]
    at org.eclipse.jetty.util.thread.SerializedInvoker$Link.run(SerializedInvoker.java:191) [jetty-util-12.0.10.jar:12.0.10]
    at org.eclipse.jetty.server.internal.HttpConnection$DemandContentCallback.succeeded(HttpConnection.java:680) [jetty-server-12.0.10.jar:12.0.10]
    at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:99) [jetty-io-12.0.10.jar:12.0.10]
    at org.eclipse.jetty.io.SelectableChannelEndPoint$1.run(SelectableChannelEndPoint.java:53) [jetty-io-12.0.10.jar:12.0.10]
    at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:979) [jetty-util-12.0.10.jar:12.0.10]
    at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.doRunJob(QueuedThreadPool.java:1209) [jetty-util-12.0.10.jar:12.0.10]
    at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1164) [jetty-util-12.0.10.jar:12.0.10]
    at java.base/java.lang.Thread.run(Thread.java:840) [?:?]

@gregw Does that ring any bell with you, or are we doing something wrong in the Spring code?

I have also asked @rstoyanchev and @simonbasle to review (my branch of) the PR, so comments from them might be forthcoming.

The next Spring Framework milestone (6.2.0-M4) is on June 13th. I am not sure if we will have resolved the test above by that point, so this PR might not make that milestone. The milestone after that is scheduled for July 11th.

simonbasle · 2024-06-06T14:59:54Z

...lux/src/main/java/org/springframework/web/reactive/socket/adapter/JettyWebSocketSession.java

+
+	private final Flux<WebSocketMessage> flux;
+	private final Sinks.One<CloseStatus> closeStatusSink = Sinks.one();
+	private final Lock lock = new ReentrantLock();


This Lock got my attention because pessimistic locking is usually not the first go-to method in Reactive Streams. Instead it's usually some sort of optimistic locking with a CAS loop.

That said, the lock protects 2 critical sections that represent quick state checks and updates. There is precedent for that sort of things, e.g. in Reactor, so in that case I think it is fine (although care will need to be exercised if the code is modified or new usage of the lock is introduced).

simonbasle · 2024-06-06T15:01:51Z

...lux/src/main/java/org/springframework/web/reactive/socket/adapter/JettyWebSocketSession.java

+				boolean demand = false;
+				this.lock.lock();
+				try {
+					this.requested += n;


this is more problematic, as n can be an "unbounded request" (Integer.MAX_VALUE) and this.requested can overflow to negative.

Suggested change

this.requested += n;

this.requested += n;

if (this.requested < 0L) {

this.requested = Long.MAX_VALUE;

}

simonbasle · 2024-06-06T15:07:29Z

...lux/src/main/java/org/springframework/web/reactive/socket/adapter/JettyWebSocketSession.java

+				try {
+					this.requested += n;
+					if (!this.awaitingMessage && this.requested > 0) {
+						this.requested--;


I'm not entirely sure how it translates in terms of getDelegate().demand() calls, but if the requested amount is unbounded, this decrement shouldn't happen.

with the above suggested change, unbounded request amount (n == Integer.MAXVALUE at any point) leads to this.requested == Long.MAX_VALUE so if that's the case the this.requested-- decrement should be skipped.

simonbasle · 2024-06-06T15:08:28Z

...lux/src/main/java/org/springframework/web/reactive/socket/adapter/JettyWebSocketSession.java

+			}
+			this.awaitingMessage = false;
+			if (this.requested > 0) {
+				this.requested--;


same as above, this should take the "unbounded request" scenario (in the Reactive Streams sense) into consideration

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

...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>

lachlan-roberts and others added 2 commits April 9, 2024 15:09

additional changes from review

cd9063b

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

Merge pull request #2 from webtide/JettyCoreHttpHandlerAdapter-WebSocket

e9cb9a9

Update spring Jetty websocket usage

poutsma self-assigned this Apr 16, 2024

jhoeller mentioned this pull request Apr 18, 2024

Add an Eclipse Jetty Core HttpHandlerAdaptor #32035

Closed

poutsma modified the milestones: 6.2.x, 6.2.0-M4 May 24, 2024

gregw and others added 3 commits May 29, 2024 14:26

Use abstract request/response classes

d6986ee

Suppress NullAway warnings for Jetty WebSocket classes

19215d4

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

fix other checkstyle warnings

364183c

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

backed out abstract response change

a76465a

upgrade jetty

gregw and others added 3 commits May 30, 2024 06:48

Merge remote-tracking branch 'origin/main' into JettyCoreHttpHandlerA…

c0cdb2b

…dapter # Conflicts: # framework-platform/framework-platform.gradle

Steps towards abstract response class

82a18ee

add explicit dependency on jetty-websocket-api

fca5d22

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

gregw added 3 commits May 31, 2024 09:00

checkstyle

17eb8df

updated response to use the abstract

38e4913

updated response to use the abstract

85b4041

Merge remote-tracking branch 'refs/remotes/webtide/JettyCoreHttpHandl…

df6ad58

…erAdapter' into JettyCoreHttpHandlerAdapter

simonbasle reviewed Jun 6, 2024

View reviewed changes

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 Feb 1, 2024

poutsma commented Apr 16, 2024

gregw commented May 23, 2024

poutsma commented May 24, 2024

poutsma commented May 28, 2024

gregw commented May 28, 2024

poutsma commented May 29, 2024

gregw commented May 29, 2024

gregw commented May 29, 2024

poutsma commented May 30, 2024

gregw commented May 31, 2024

poutsma commented May 31, 2024 •

edited

poutsma commented Jun 6, 2024

simonbasle Jun 6, 2024

simonbasle Jun 6, 2024 •

edited

simonbasle Jun 6, 2024

simonbasle Jun 6, 2024

simonbasle Jun 6, 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

Are you sure you want to change the base?

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 Feb 1, 2024

Choose a reason for hiding this comment

poutsma commented Apr 16, 2024

gregw commented May 23, 2024

poutsma commented May 24, 2024

poutsma commented May 28, 2024

gregw commented May 28, 2024

poutsma commented May 29, 2024

gregw commented May 29, 2024

gregw commented May 29, 2024

poutsma commented May 30, 2024

gregw commented May 31, 2024

poutsma commented May 31, 2024 • edited

poutsma commented Jun 6, 2024

simonbasle Jun 6, 2024

Choose a reason for hiding this comment

simonbasle Jun 6, 2024 • edited

Choose a reason for hiding this comment

simonbasle Jun 6, 2024

Choose a reason for hiding this comment

simonbasle Jun 6, 2024

Choose a reason for hiding this comment

simonbasle Jun 6, 2024

Choose a reason for hiding this comment

poutsma commented May 31, 2024 •

edited

simonbasle Jun 6, 2024 •

edited