Skip to content

Conversation

@zUniQueX
Copy link
Contributor

@zUniQueX zUniQueX commented Jul 19, 2023

Supersedes #5342

This PR provides an early preview of the Jetty 12 upgrade with Jetty 12.0.0.beta4.

Jersey 3.1.x currently has compatibility issues with Jetty. Jersey 3.1 is compatible with Jakarta EE 10 and therefore uses servlet-api 6. But the current Jetty version 11.0.x isn't compatible with servlet-api 6.

Although the upgrade to Jetty 12 is a major upgrade, this can be released for the servlet compatibility in Jersey 3.1.x IMHO.

Notable changes:

  • The servlet-api isn't integrated into Jetty's core classes any more and rather moved out to a separate module. For servlet classes the correct servlet module has to be included (e.g. jetty-ee10-servlet for Jersey 3.1).
    The JettyHttpContainer is rewritten by removing all servlet related classes. This prevents incompatibilities with future servlet versions.
  • The Handler interface has changed and now uses a Callback class to signal output completion. The handled state is set with the method's return value.
  • JettyHttpContainer: The SecurityContext is set via the AuthenticationState. In the current implementation no SecurityHandler is provided and therefore the AuthenticationState will always be null. Maybe the JettyHttpContainer should implement the Handler.Singleton interface to let users specify a SecurityHandler.

@zUniQueX zUniQueX force-pushed the jetty-12-multirelease branch 3 times, most recently from e658ae1 to 2444dd1 Compare July 27, 2023 18:07
Signed-off-by: Steffen Nießing <zuniquex@protonmail.com>
Signed-off-by: Steffen Nießing <zuniquex@protonmail.com>
@zUniQueX zUniQueX force-pushed the jetty-12-multirelease branch from 39714c1 to fd44001 Compare August 8, 2023 17:48
@zUniQueX
Copy link
Contributor Author

zUniQueX commented Aug 8, 2023

The final stage of Jetty 12.0.0 was released yesterday. So I've updated the PR to the released version. Since beta4 no significant changes were introduced.

@zUniQueX
Copy link
Contributor Author

@senivam Thanks for re-triggering the Jenkins build. Unfortunately, looks like the OSGI test is unstable.

@senivam
Copy link
Contributor

senivam commented Aug 15, 2023

@zUniQueX I think that's not directly OSGi, but connectivity between ci.eclipse.org/jersey JIPP and eclipse maven repo. It always fails for connectivity reasons.

@zUniQueX
Copy link
Contributor Author

@senivam Thanks for clarification 👍

@senivam
Copy link
Contributor

senivam commented Aug 15, 2023

@zUniQueX you are welcome :) actually, I've retriggered the build again. But it waits in the queue because several builds are already running and this one will run after them. Let's hope it will finish OK.

@jansupol
Copy link
Contributor

@zUniQueX Thank you for this effort. Glad to see Jetty 12 is final, yet.

Pros: We definitely want this. ASAP.
Cons: No JDK 11 support. Different Jetty package names.

Open question: Can we do something for people who want latest Jersey 3.1, but JDK 11? Perhaps clone the existing Jetty module into some Jetty-Jdk11 or Jetty11 module?

@zUniQueX
Copy link
Contributor Author

@jansupol I don't know, if that's a good option. Then we would need a downgrade of the servlet api to 5.x because Jetty 11 is EE 9 only. This may cause compatibility issues with other components.

We could check, if Jersey's tests pass in such a module to support common cases, but in some edge cases this will probably fail.

@jansupol
Copy link
Contributor

@zUniQueX My concern was mainly regarding the client - connector. Jetty Client is more widely used than Jetty Server.

Similarly, we do have Apache connector and Apache5 connector.
We will need to create Netty 5 connector, soon.
Perhaps we should have Jetty12 connector. The difference, though, is that Jetty version is bound with Jakarta EE versions. Jetty 12 is EE 10 Jetty, but Jetty is behind EE10 release, and people are using Jetty 11 client with Jersey. I am not sure whether we should have Jetty11 or Jetty12 module, but I am positive there should be a module supporting Jetty 11 client. Will see what other people think about it.

@zUniQueX
Copy link
Contributor Author

@jansupol That makes sense to me.

I'd suggest to use the BOM-managed versions in the modules without suffixes, so I'd go for a jersey-jetty11-connector module with the current code.

@zUniQueX
Copy link
Contributor Author

zUniQueX commented Sep 2, 2023

@jansupol I've added a module for the Jetty 11.x client. I think that's the better idea than shipping with Jetty 11 in the default Jetty connector module.

To add this connector properly I had to switch to the Grizzly2 container to prevent artifact clashes. That's also the reason why I've skipped adding the new connector to the e2e tests.

One test fails with the Grizzly2 container but I can't understand why. A higher timeout fixes the issue. However, the current timeout should be high enough. I've temporarily disabled the test since we know the connector works because I've just copied the existing code.

Copy link
Contributor

@jansupol jansupol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@senivam senivam merged commit 521015e into eclipse-ee4j:3.1 Sep 7, 2023
@senivam senivam added this to the 3.1.4 milestone Sep 7, 2023
@zUniQueX zUniQueX deleted the jetty-12-multirelease branch September 10, 2023 13:18
@zUniQueX
Copy link
Contributor Author

@jansupol Do you already have plans for the next release? Our EE10 upgrade at dropwizard is currently blocked by a release of this PR.

@jansupol
Copy link
Contributor

We are finishing, but it won't happen this week.

@zUniQueX
Copy link
Contributor Author

Okay, thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants