-
Notifications
You must be signed in to change notification settings - Fork 358
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
Jetty 12 multirelease #5372
Jetty 12 multirelease #5372
Conversation
e658ae1
to
2444dd1
Compare
Signed-off-by: Steffen Nießing <[email protected]>
Signed-off-by: Steffen Nießing <[email protected]>
39714c1
to
fd44001
Compare
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. |
@senivam Thanks for re-triggering the Jenkins build. Unfortunately, looks like the OSGI test is unstable. |
@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. |
@senivam Thanks for clarification 👍 |
@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. |
@zUniQueX Thank you for this effort. Glad to see Jetty 12 is final, yet. Pros: We definitely want this. ASAP. 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? |
@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. |
@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. |
@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 |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@jansupol Do you already have plans for the next release? Our EE10 upgrade at dropwizard is currently blocked by a release of this PR. |
We are finishing, but it won't happen this week. |
Okay, thanks 👍 |
Jersey/Jetty, at least in the 3.1 version line, creates one thread for each HTTP request. This behavior was introduced with eclipse-ee4j#5372 and seems not present in the 2.x or 3.x versions of Jersey. From the javadoc of `java.util.Timer`: ``` Implementation note: All constructors start a timer thread. ... After the last live reference to a Timer object goes away and all outstanding tasks have completed execution, the timer's task execution thread terminates gracefully (and becomes subject to garbage collection). However, this can take arbitrarily long to occur. ``` It is fair to assume that "arbitrarily long" may also mean _never_, in case GC never runs. This change replaces the timer & thread per request with a `ScheduledExecutorService` instance per `JettyHttpContainer`. Also changed the set-timeout mechanism to use `System.nanoTime()` instead of `System.currentTimeMillis()`, because the latter is prone to wall-clock drift and can result into wrong timeout values. Fixes eclipse-ee4j#5588 Signed-off-by: Robert Stupp <[email protected]>
Jersey/Jetty, at least in the 3.1 version line, creates one thread for each HTTP request. This behavior was introduced with eclipse-ee4j#5372 and seems not present in the 2.x or 3.x versions of Jersey. From the javadoc of `java.util.Timer`: ``` Implementation note: All constructors start a timer thread. ... After the last live reference to a Timer object goes away and all outstanding tasks have completed execution, the timer's task execution thread terminates gracefully (and becomes subject to garbage collection). However, this can take arbitrarily long to occur. ``` It is fair to assume that "arbitrarily long" may also mean _never_, in case GC never runs. This change replaces the timer & thread per request with a `ScheduledExecutorService` instance per `JettyHttpContainer`. Also changed the set-timeout mechanism to use `System.nanoTime()` instead of `System.currentTimeMillis()`, because the latter is prone to wall-clock drift and can result into wrong timeout values. Fixes eclipse-ee4j#5588 Signed-off-by: Robert Stupp <[email protected]>
Jersey/Jetty, at least in the 3.1 version line, creates one thread for each HTTP request. This behavior was introduced with #5372 and seems not present in the 2.x or 3.x versions of Jersey. From the javadoc of `java.util.Timer`: ``` Implementation note: All constructors start a timer thread. ... After the last live reference to a Timer object goes away and all outstanding tasks have completed execution, the timer's task execution thread terminates gracefully (and becomes subject to garbage collection). However, this can take arbitrarily long to occur. ``` It is fair to assume that "arbitrarily long" may also mean _never_, in case GC never runs. This change replaces the timer & thread per request with a `ScheduledExecutorService` instance per `JettyHttpContainer`. Also changed the set-timeout mechanism to use `System.nanoTime()` instead of `System.currentTimeMillis()`, because the latter is prone to wall-clock drift and can result into wrong timeout values. Fixes #5588 Signed-off-by: Robert Stupp <[email protected]>
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 withservlet-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:
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.Handler
interface has changed and now uses aCallback
class to signal output completion. Thehandled
state is set with the method's return value.JettyHttpContainer
: TheSecurityContext
is set via theAuthenticationState
. In the current implementation noSecurityHandler
is provided and therefore theAuthenticationState
will always benull
. Maybe theJettyHttpContainer
should implement theHandler.Singleton
interface to let users specify aSecurityHandler
.