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
Comment From: pivotal-cla
@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.
Comment From: gregw
@pivotal-cla Working on getting a CCLA signed. Stand by....
Comment From: gregw
@lachlan-roberts can you sign the individual CLA.
Comment From: gregw
This PR is failing 2 tests that undertow also fails: See https://github.com/spring-projects/spring-framework/issues/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...
Comment From: pivotal-cla
@gregw Thank you for signing the Contributor License Agreement!
Comment From: gregw
@jhoeller Is there anything process wise that we need to do to move this PR on? It would be good to get some review to make sure we are on the right path.
Comment From: gregw
@jhoeller @bclozel @sbrannen What can we do to move this PR on? I'm sure there is lots that we need to update, but we need review to know what that is.
Comment From: poutsma
@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.
Comment From: gregw
@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.
Comment From: poutsma
@gregw Fair point. I had to take some unforeseen medical leave, but plan to focus on this PR this coming week.
Comment From: poutsma
@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.
Comment From: gregw
@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
JettyCoreServerHttpRequestandJettyCoreServerHttpResponseimplement theServerHttpRequestandServerHttpResponseinterfaces, and not to extend the abstract base classes? It appears that a lot of changes in the PR derive from this decision (for instancegetNativeRequestis 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.
Comment From: poutsma
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?
Comment From: gregw
@poutsma my step 1 is pretty broken. I think it need more work. stand by....
Comment From: gregw
@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...
Comment From: poutsma
@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.
Comment From: gregw
@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.
Comment From: poutsma
@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.
Comment From: poutsma
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.
Comment From: gregw
My apologies for being slow now... I've seen your reviews and will respond in the next few days.
Comment From: gregw
@poutsma Our branches have diverged as I made the changes to use AbstractServerHttpRequest and AbstractServerHttpResponse in my branch and you forked before that. Let me see if I can rebase yours onto mine....
Comment From: gregw
Actually, I'm not exactly sure why our branches were seen as divergent. I've merged yours back to mine and made sure that there are no differences, and also merged to lastest origin/main. I'm not 100% sure the merge is correct, so we may want to proceed with your branch, but at least we can see your changes in this PR and I can work with it as well. Now looking at the test failure....
Comment From: gregw
@poutsma
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 ...@gregw Does that ring any bell with you, or are we doing something wrong in the Spring code?
I think it is likely a problem with the jetty ContentSourcePublisher, for which we already have a PR in review: https://github.com/jetty/jetty.project/pull/11849
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.
Agreed we are not making the 13th. We have a jetty release at the end of the month, so I will get #11849 merged for that and also look at change @rstoyanchev made.
@lachlan-roberts can you look at the websocket reviews before end of month, so that any changes needed in next jetty release can be included. E.g. anything needed to get the Container without the Handler
Comment From: gregw
The MultipartWebClientIntegrationTests test did not fail running from command line with jetty 12.0.11-SNAPSHOT
Comment From: poutsma
The
MultipartWebClientIntegrationTeststest 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.
Comment From: lachlan-roberts
@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.
Comment From: simonbasle
@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 👍
Comment From: gregw
@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.
Comment From: simonbasle
@lachlan-roberts unless I've missed something, you haven't yet signed the CLA have you?
Comment From: lachlan-roberts
@simonbasle yes I have already signed the CLA.