Support Jetty 11 #186
Support Jetty 11 #186
Conversation
The super-interface LifeCycleListener added default methods. The adapter became redundant.
The Gradle module is called 'filterWebapp' on the filesystem. Mind the lowercase 'a'! In Jetty 11, this caused Gretty to fail to locate the filter configuration.
The FilteringClassLoader implements the server classes concept, which Jetty appears to support natively. I also cleaned up the list of server class patterns, because I cannot figure out what exactly 'groovyjarjarantlr' and the likes are supposed to match on.
Sharing the session handler between contexts leads to an NPE, because startup of the first context now reaches into the lifecycle of the second webapp, which has not started yet. SessionHandler objects, which carry context-specific lifecycle information, must not be shared between contexts.
|
@f4lco - thank you, yet again, for your hard work. I'm not sure how you can drink all the beer we owe you! 😄 You mention On your points:
Otherwise - amazing work as always! Thank you and feel free to merge when you want! |
|
Oh, another thing I forgot. A lot of this code will be the same for Jetty 10, right? Perhaps it can be extracted in |
Many thanks 😄
Yes, that was along the way to this PR. Have a look at the commit history - almost every commit is inspired by some sort of test failure. Actually, I was pleasantly surprised how much signal I got out of the test suite. I supposed they either fail all together or none. In reality however, many test cases exercised the code in a unique way leading to unique root causes & distinct fixes. Regarding the other points: that's basically what I was thinking - there's nothing to be done right now. I turned on notifications on the logback ticket you created, just in case. Thanks for that.
Some sort of. I do know that a couple of patches apply to Jetty 10. However, they do not have a reasonable location on master. One would have to create the 'gretty-runner-jetty10' module in master first, and then possibly apply a subset of patches from this branch on top of it.
This won't work for two reasons: first, as per comment above, the file directories are distinct, mostly it's not shared code between Jetty 10 and 11 that is affected. Second, I gave up 'rebase' strategy, and starting cherry-picking relevant commits from master a while ago. There have a been a couple of conflicts, changes which do not have an equivalent in grett4 because they affect older runners, and even changes which had already been done to gretty4. Time to remember those conflict resolutions & stabilize. When you cherry-pick and the build goes awry, it's easy to pinpoint and fix. Thank you for your review! |
|
@f4lco - I wasn't sure where to ping you so here is as good as it gets I guess. I saw you working on a Jetty 10 branch a few days ago which later disappeared. What was up with that? I still have it locally if you've deleted it by accident. 😄 |
|
@boris-petrov wow I guess my "work" does get noticed 😆 Yeah, eventually, I gave up on it, because I realized without shifting requirements in a major way, that patch isn't going to get merged. This is what the current compatibility looks like:
... and Jetty 10 falls right in the upper corner, supporting javax, but having a minimum JDK requirement of 11. So I am open to your suggestions to 'rescue' the patch, whether it's shifting requirements, or a technical solution. If you have a technical solution to the dilemma, I'd be glad to hear you out. I remember not reaching a definitive agreement on #161. |
|
@f4lco - I was under the impression that Jetty 10 would be working on Java 8. If that's not so, I see no point in supporting it. The migration from Java 8 to Java 11 is a much bigger burden on people than renaming a bunch of imports from P.S. As for the branch - I restored the last version that I have. It's free to be there so let it stay for now. :) Thanks for the work! |
|
Yes, I think you are correct, and we are on the same page here 👍 |
I made Jetty 11 work. It was really a lot of work and many test failures. I do not expect the changes to be overly controversial, but here are a few talking points:
jakarta.*really a "system class"? This fixes a set of classloading issues in a logical way, but I wonder if they are indeed system classes, why isn't that enforced in Jetty by default? It also does not make sense to compile your app against one version ofjakarta-servlet-api, and deploy to a server which ships with a different version. See 9d4bc4b.