Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an Eclipse Jetty Core HttpHandlerAdaptor #32035

Closed
gregw opened this issue Jan 16, 2024 · 10 comments
Closed

Add an Eclipse Jetty Core HttpHandlerAdaptor #32035

gregw opened this issue Jan 16, 2024 · 10 comments
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: superseded An issue that has been superseded by another type: enhancement A general enhancement

Comments

@gregw
Copy link
Contributor

gregw commented Jan 16, 2024

Affects: >= 6.1.4

The current Jetty integration uses the Servlet abstraction, which is primarily blocking and has additional buffering layers.

The jetty core server (added in jetty-12) has a reactive style asynchronous API very compatible with Flux<DataBuffer> style programming. A simpler and more efficient integration can thus be created by avoiding Servlets and going direct to jetty core.

@gregw
Copy link
Contributor Author

gregw commented Jan 16, 2024

Work has commenced on this enhancement at: webtide#1
However, we'd very much appreciate any early feedback/advice as we are not familiar with spring internals and conventions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 16, 2024
@sbrannen sbrannen changed the title Add an Eclipse Jetty Core HttpHandlerAdaptor Add an Eclipse Jetty Core HttpHandlerAdaptor Jan 16, 2024
@sbrannen sbrannen added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Jan 16, 2024
@jhoeller jhoeller added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jan 16, 2024
@jhoeller jhoeller added this to the 6.2.x milestone Jan 16, 2024
@jhoeller
Copy link
Contributor

Hi Greg,

Thanks for that suggestion and the initial work! This looks sensible indeed for providing even closer Jetty 12 integration. And since Jetty's Servlet adapters live entirely outside of the core in version-specific packages and modules now, it is also attractive from our maintenance perspective to not involve those modules in WebFlux anymore.

In terms of our timeline, this would be a great fit with our 6.2 for which we intend to do the first milestone in May, leading up to general availability in November. We are happy to review and test the current prototype ASAP, and once this shapes up into a PR, to merge it into the mainline for inclusion in 6.2 M1.

@gregw
Copy link
Contributor Author

gregw commented Jan 17, 2024

@jhoeller currently we are implementing it as an alternative to the Jetty Servlet adaptor, as I'm not clear if there are ways to surface the underlying servlet request/response in spring applications or not??

Would it be reasonable to keep the servlet based adaptor as a deprecated for a release or two?

That timeline sounds good... I'd like to think we'd be ready before that... but January is almost over already!

@jhoeller
Copy link
Contributor

In WebFlux, the Servlet request/response does not leak through to the application at all, so migrating from the Servlet-based adapter to the Jetty Core adapter should be entirely transparent to WebFlux users in a Spring Boot auto-configuration context.

We could suggest a switch to the Jetty Core adapter by default for Spring Boot 3.4 (GA in November). So whenever someone indicates Jetty for WebFlux then, they would simply get the Jetty Core adapter as of Boot 3.4, just like they got the Jetty 11->12 upgrade with Boot 3.2 a few months ago. I see no need to make this an explicit choice for Spring Boot users, rather treating it as an internal upgrade as of a particular Boot release. Do you see any downside to the Jetty Core adapter (in a matured state)?

Keeping the Servlet-based adapter around for programmatic setups seems sensible in Spring Framework 6.2 but we'd probably drop it entirely as of 7.0.

@gregw
Copy link
Contributor Author

gregw commented Jan 18, 2024

@jhoeller I've labelled the jetty Handler as non-blocking, which allows jetty to make some more efficient scheduling decisions (calling handler with hot cache) even when low on threads. However, this is only safe it the handler really is non blocking. What is the expectation that a spring HttpHandler really is non blocking? If it is always async, I can leave it as is, but if they can sometimes block, then I can make it configurable... else if they frequently block, then I will just label as blocking.

@jhoeller
Copy link
Contributor

@gregw a general non-blocking label should be fine there since this is exclusively for WebFlux as a reactive stack. Any kind of blocking in that stack would be a mistake unless it got scheduled on a different dispatcher. In any case, not a scenario the HttpHandler has to deal with.

@gregw
Copy link
Contributor Author

gregw commented Jan 31, 2024

@jhoeller FYI we are making good progress. However, we are failing two tests that undertow is also failing (and currently excludes). 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...

@gregw
Copy link
Contributor Author

gregw commented Jan 31, 2024

In WebFlux, the Servlet request/response does not leak through to the application at all, so migrating from the Servlet-based adapter to the Jetty Core adapter should be entirely transparent to WebFlux users in a Spring Boot auto-configuration context.

@jhoeller I'm trying to square your response here with the comment in jetty/jetty.project#11342 (comment) that suggests a springboot integration has been done by extending jakarta.servlet.

If we remove the servlet based jetty integration, will that prevent applications from using Servlet if they need to?

@jhoeller
Copy link
Contributor

jhoeller commented Feb 5, 2024

@gregw I suppose the user who reported that Jetty issue uses Spring MVC on a Servlet stack, or even a custom Servlet arrangement on a similar stack, but not WebFlux.

There is a duality between Servlet-based stacks with Spring MVC or similar Servlets, with the Servlet API being a user-visible concern - as opposed to Spring WebFlux (our reactive web stack) which is entirely self-contained, with Servlet adaptation being an internal detail that does not leak through. Even with a rewritten Jetty adapter underneath Spring WebFlux, people can always choose to use a Servlet stack instead, deploying either a war file or with an embedded Servlet stack.

In any case, the HttpHandler in the works here only needs to cover Spring WebFlux (where the Servlet API does not leak through). It won't be in use for a regular Spring MVC stack (where the Servlet API is user-visible) at all since such a Servlet stack will be deployed in a traditional fashion instead.

@bclozel bclozel changed the title Add an Eclipse Jetty Core HttpHandlerAdaptor Add an Eclipse Jetty Core `HttpHandlerAdaptor Feb 14, 2024
@bclozel bclozel changed the title Add an Eclipse Jetty Core `HttpHandlerAdaptor Add an Eclipse Jetty Core HttpHandlerAdaptor Feb 14, 2024
@jhoeller
Copy link
Contributor

Superseded by PR #32097.

@jhoeller jhoeller removed this from the 6.2.x milestone Apr 18, 2024
@jhoeller jhoeller added the status: superseded An issue that has been superseded by another label Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: superseded An issue that has been superseded by another type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants