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

API compatibility between Jetty 11 and 12: org.eclipse.jetty.server.Request/Response#getHeaders() #8938

Closed
jhoeller opened this issue Nov 25, 2022 · 8 comments
Assignees
Labels
Enhancement Stale For auto-closed stale issues and pull requests

Comments

@jhoeller
Copy link

jhoeller commented Nov 25, 2022

For Spring Framework 6.0's core Jetty integration in WebFlux, we interact with org.eclipse.jetty.server.Request#getHttpFields() and org.eclipse.jetty.server.Response#getHttpFields() in Jetty 11 which changed to #getHeaders() in Jetty 12.

Trying to avoid reflection for supporting both versions of Jetty, is there any chance you could keep providing #getHttpFields() in Jetty 12 (in deprecated form) or alternatively - even preferably? - introduce #getHeaders() in a Jetty 11.0.x release as well, allowing integrators like Spring to have a single method to call between Jetty 11.0.x and 12 there?

@jhoeller jhoeller changed the title Compatibility between Jetty 11 and 12: org.eclipse.jetty.server.Request Compatibility between Jetty 11 and 12: org.eclipse.jetty.server.Request#getHeaders() Nov 25, 2022
@jhoeller jhoeller changed the title Compatibility between Jetty 11 and 12: org.eclipse.jetty.server.Request#getHeaders() API compatibility between Jetty 11 and 12: org.eclipse.jetty.server.Request/Response#getHeaders() Nov 25, 2022
@sbordet
Copy link
Contributor

sbordet commented Nov 25, 2022

@jhoeller we can certainly introduce Request.getHeaders() in Jetty 11, but would that be all you need to integrate also Jetty 12?

Can you point out what Spring code exactly you refer to, so we can take a look as well and perhaps suggest alternatives, if any?

@jhoeller
Copy link
Author

jhoeller commented Nov 26, 2022

@sbordet that would be all we need indeed, on both Request and Response. We would simply require Jetty 11.0.1X as a minimum then (whenever those new methods get introduced), also compiling the framework against that version, and seamlessly support Jetty 12 at runtime with the same arrangement. There might also be other integrators who could benefit from such early availability of getHeaders(), with getHttpFields() possibly getting deprecated in 11.0.x and phasing out rather than being suddenly replaced from one generation to the next.

Concretely, this is about the JettyHttpHandlerAdapter for WebFlux where we obtain Jetty's HttpFields for adapting them to the WebFlux request and response APIs. See lines 110 and 147 where we currently apply a reflective workaround for a getHeaders() call on Jetty 12 while using getHttpFields() below on Jetty 11. Note that these Jetty 12 support efforts for WebFlux are very recent and not released yet but meant to be included in Spring Framework 6.0.3 in mid December, for early support of Jetty 12 beta releases - which I suppose could be coming soon?

@sbordet sbordet self-assigned this Nov 28, 2022
@sbordet
Copy link
Contributor

sbordet commented Nov 28, 2022

@jhoeller I gave this a try, but I don't think it will work if I understand correctly your comment.
The problem is that in Jetty 11, Request is a class, while in Jetty 12 is an interface.

Even providing the getHeaders() methods, if you compile Spring again Jetty 11, but then try to run it with Jetty 12, you will get:

Exception in thread "main" java.lang.IncompatibleClassChangeError: Found interface org.eclipse.jetty.server.Request, but class was expected

You would have to recompile Spring against Jetty 12, or rely on reflection in both cases (you have a single reflection on getHeaders() though).

Just to state our ideal setup in a near future, we would like -- if possible -- Spring 6.1.x to depend on Jetty 12 only, once Jetty 12 is out in stable versions.
This would allow Spring to depend only on Jetty APIs, not on Servlet APIs, for the Jetty integration, similar to your ReactorNetty2HttpHandlerAdapter, so that you also have Jetty implementation that is non-blocking and reactive.

For Spring 6.0.x I understand that you support both Jetty 11 and Jetty 12 via Servlet APIs, which is fine, but unfortunately requires reflection due to the API incompatibilities.

Also, writing to the OutputStream in Spring 6.0.x is probably still best done without casting to HttpOutput, again due to API incompatibilities.

I will prepare a PR to add getHeaders() to Jetty 11, so at least you have to reflect just one method.

Let us know what you think about dropping support for Jetty 11 in a near-future Spring version.

sbordet added a commit that referenced this issue Nov 28, 2022
… for Spring.

Introduced Request.getHeaders() and Response.getHeaders().

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@jhoeller
Copy link
Author

Oh, thanks for pointing that out! I only recompiled against Jetty 12, have not actually tried compiling on 11 and running against 12 yet. If Request and Response are interfaces now, JettyHttpHandlerAdapter might have to use reflection for any access to their methods for runtime compatibility purposes then. We can live with such reflection if it is a temporary state of affairs but would probably require Jetty 12 for WebFlux in Spring Framework 6.1 indeed then.

We could rather easily keep up Jetty 11 support for plain Servlet purposes with Spring MVC/WebSocket in 6.1 (since we do not need Jetty 12 specific APIs there), while at the same time requiring Jetty 12+ for embedded HTTP engine purposes in WebFlux. So we would not drop Jetty 11 support completely but rather just drop support for using Jetty 11 as an embedded engine in our reactive web stacks, for the benefit of deeper integration with the Jetty 12 core.

In the meantime, we are considering basic Jetty 12 support in Spring Boot 3.1 (May 2023), based on Spring Framework 6.0.x still. Is there a chance of Jetty 12 going final in that timeframe? If not, we might ship such early support as a preview only, before jumping to Jetty 12 in Spring Boot 3.2 / Framework 6.1 (November 2023).

@jhoeller
Copy link
Author

Looking at this more closely, it seems straightforward enough to address early Jetty 12 compatibility for our purposes since JettyHttpHandlerAdapter does most of its business through the Servlet API anyway and only really needs to call those getHeaders() methods on Request/Response, so it is just two points of reflection. We can even keep calling the corresponding getHttpFields() methods straight on Jetty 11, so don't really need a backport of the getHeaders() methods to Jetty 11.0.x then - if we have to use reflection for the time being anyway (for the class->interface adaptation). So we can probably skip #8941, unless you see a general benefit in it for other purposes. Thanks for your efforts there, in any case!

The only other concern is unwrapping a given HttpServletRequest/Response to a Jetty Request/Response which seems to require a ServletContextRequest.getServletContextRequest/ServletContextResponse.getBaseResponse call on Jetty 12 since we cannot just downcast from the Servlet API types anymore. We can detect and do that in JettyHttpHandlerAdapter for the time being as well. I'm just wondering about the different method names there, is there a specific reason why the latter is not named getServletContextResponse but rather getBaseResponse?

Overall, for an early Jetty 12 compatibility arrangement (within the Servlet based adapter in WebFlux) while retaining a Jetty 11 baseline, this is probably good enough for Spring Framework 6.0.x. So we can close this getHeaders issue from my perspective, or repurpose it for the getServletContextRequest vs getBaseResponse part above.

Beyond that, for a tighter Jetty 12 based engine in WebFlux, we would not build on the Servlet-derived JettyHttpHandlerAdapter at all anymore but rather ship a plain Jetty API based variant instead that does not touch the Servlet API at all. However, that would be a Spring Framework 6.1 effort with a November 2023 target.

@sbordet
Copy link
Contributor

sbordet commented Nov 29, 2022

In the meantime, we are considering basic Jetty 12 support in Spring Boot 3.1 (May 2023), based on Spring Framework 6.0.x still. Is there a chance of Jetty 12 going final in that timeframe?

Our goal is betas in December/January, and final in February/March. Integration with Spring Boot would be interesting indeed, and one more use case for us to validate Jetty 12.

Note that Jetty 12 will be able to do "core" (meaning no Servlet dependencies), EE9 and EE10, so you will be able to retain the Jetty Servlet integration if you want, but also offer a more performant "core" alternative.

I'm just wondering about the different method names there, is there a specific reason why the latter is not named getServletContextResponse but rather getBaseResponse?

Thanks for pointing that out, we will fix it to getServletContextResponse().

Beyond that, for a tighter Jetty 12 based engine in WebFlux, we would not build on the Servlet-derived JettyHttpHandlerAdapter at all anymore but rather ship a plain Jetty API based variant instead that does not touch the Servlet API at all. However, that would be a Spring Framework 6.1 effort with a November 2023 target.

Agreed.

I'll drop #8941 then.

Please reach out for any issue or help with the integration in Spring Boot. Thanks!

Copy link

This issue has been automatically marked as stale because it has been a
full year without activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@github-actions github-actions bot added the Stale For auto-closed stale issues and pull requests label Nov 30, 2023
@sbordet
Copy link
Contributor

sbordet commented Dec 9, 2023

Closing as Spring 6.1 and Spring Boot 3.1 have now support for Jetty 12.

@sbordet sbordet closed this as completed Dec 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Stale For auto-closed stale issues and pull requests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants