-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
ClassNotFoundException when using jetty 9.4.0.M0 within OSGi container #705
Comments
9.4.0 is not a stable branch (yet). That Loader change was very important to us, and its highly unlikely that it will be reverted. Do you have a relevant testcase on the thermostat project we can use to replicate? |
@joakime There is not exactly an easy test case, unfortunately. To my knowledge it requires a jetty server to be started from within an OSGi framework. The best we have is something like this:
|
@jerboaa That loader code was rather evil as it went on a rather full search of every possible classloader looking for a class to load. This is rather against the spirit of OSGi and all other classloader architectures. We want to run properly in OSGi by design, not by bypassing classloaders. I see that @janbartel has self assigned this issue, so she will be looking into what we need to do to make the OSGI run correctly. |
@jerboaa actually at the point in time that the configuration classes are loaded, there is NO thread context classloader. Thus, the Loader will use Class.forName() call, which will use the classloader of the Loader class, which will be the jetty-util bundle, unfortunately. So, the easy fix for this is to set up a thread context classloader in thermostat's JettyContainerLauncher, and ensure that it points to the classloader of the JettyContainerLauncher class, which presumably has the right bundle import statements to resolve all of the org.eclipse.jetty.* classes. The right place to do that would be around line 142, just before you call new Server(). This is what we do in the jetty osgi support classes: in our bundle listener that detects and deploys new webapp bundles, we set a thread context classloader that is the classloader of the main jetty bundle, which is able to resolve all jetty classes: https://github.com/eclipse/jetty.project/blob/jetty-9.4.x/jetty-osgi/jetty-osgi-boot/src/main/java/org/eclipse/jetty/osgi/boot/BundleWebAppProvider.java#L110. Actually the main jetty bundle also uses a Dynamic-Import statement to resolve all org.eclipse.jetty.* classes, but from the looks of the relevant thermostat manifest, I think you should have the right import statements to be able to resolve everything necessary. |
@janbartel I can do this and it'll likely work, but I don't understand why it should be a third-party's (Thermostat in our case) responsibility to set a TCCL so as to facilitate proper class loading across jetty bundles. In this particular instance the caller, |
Use regular class loading for default configuration instance creation.
@janbartel --^ is a quick hack for circumventing Loader class loading which works for us. I believe it shouldn't break anything else either since it special cases the default config case. But that's hard for me to judge/test. Anyway, something like that would be more like the OSGi way ;-) |
http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=3086#c2 Has a patch which works around this issue. Not nice :-( |
@jerboaa the problem with the proposed fix is that it only takes care of the bare minimum set of Configurations that happen to be defined in the jetty-webapp bundle - it won't work if any Configurations from other bundles are added in (eg AnnotationConfiguration, EnvConfiguration, PlusConfiguration etc etc). So I think any solution needs to work for the general case, not just this specific one. While I understand your sentiment that you would like this to work out-of-the-box in an OSGi environment, it's very difficult to make that work ;) I will have a further ponder on what more we could do to make this easier, but I don't think any solution could be entirely transparent to the embedding application: fundamentally only the application knows about a classloader that has been wired up (via manifest import statements etc) with visibility across jetty classes, and it is that classloader that should be the parent of the webapp's classloader, and it is also that classloader that needs to be used to load the Configurations, before the webapp's classloader has been established. I'll take a look and see if there's any refactoring that I can do to our existing osgi support (jetty-osgi-boot bundle) to extract some utilities that would make it easier to use jetty in osgi without using our entire solution to deploy jetty as an osgi service. |
Which makes me wonder why the Loader call which takes a class as additional param is the wrong call though. Failing that, if jetty-util would have proper bundle wiring via Import-Package it should be able to resolve classes via |
@jerboaa Loader.loadClass(Class,String) wouldn't work because you don't know what class to pass in that has a classloader that can load the desired class. If you passed in WebAppContext.class, then sure you can load classes that are in the jetty-webapp bundle, but you couldn't load classes that are in eg jetty-annotation bundle. resolution:="optional" is not really a solution either for a couple of reasons:
Fundamentally, it just doesn't work in osgi-land to have a bundle try and load classes on behalf of all other bundles: it contradicts the very essence of osgi :) |
Right, makes sense. Any recommendations on how to correctly use embedded jetty within an OSGi context? My fear is that we might run into an issue like this again on the next version update. I suppose setting the TCCL before any of the jetty classes are being used is the only choice? Note that in Thermostat-land we needed to add a delegating class loader so as to be able to continue to resolve system classes coming from the JDK. It's pretty ugly: |
@jerboaa not sure why the WorkAroundJettyClassLoader is necessary? If you set the TCCL to be the classloader for JettyContainerLauncher that should be enough I would think: the osgi environment never looks at the TCCL, only jetty uses it when doing explict classloading and I would have thought that system classes would be loaded implicitly (and therefore from the osgi environment loader). Can you post an example of what fails unless you have this fix in place? For the jetty osgi library, for every jetty Server instance we create a new URLClassloader whose parent is the classloader of the bundle with all the correct import statements. Let's call that the "container classloader". When a webapp is deployed, the TCCL is set to the container classloader, and it becomes the parent of the webapp's classloader that is created during deployment. At no time do we do any type of explict delegation looking for the system classloader. |
Example of the exception we see when not using WorkAroundJettyClassLoader:
Thinking some more about this I've got some ideas as to how to simplify our class loading maze a bit. TCCL becoming the parent of WebAppClassLoader was the hint :-) Hopefully this will work with jetty 8+. I agree, WorkAroundJettyClassLoader, won't be needed. |
So here is how we ended up solving this webapp container vs. OSGi class loading issue: We still delegate to the system classloader since by setting the TCCL all classes of the webapp will get loaded by WeppAppClassLoader delegating to the set TCCL loader as parent. Our webapp libs include mongo-java-driver which uses java system classes and we don't know which they're using unless we try those empirically and solve it via explicit Import-Package instructions. That would be a fairly brittle approach. The delgating-to-system-loader-if-not-found seems the better alternative. Either way, I think you can close this issue. |
Ok, thanks for that info. Closing this issue. |
hi @janbartel and @joakime I still reproduce #262 with latest of jetty 9.4.1.v20170120 (setup a http2 on osgi container equinox) from comment of janbartel
for Osgi container, BundleWebAppProvider already set TCCL so i don't know what i have to do to resolve #262. please let me know latest of jetty support out of box to setup http2 on Osgi container or point me how to fix #262 |
After finding this issue, and setting the Thread ContextClassLoader I'm left with
Just changed my target from Neon to Oxygen in Eclipse, hence upgrading from jetty 9.3 to 9.4 and my osgi embedded war file stopped working .... really not liking this too .... |
Hi @janbartel , Is this issue fixed in Jetty Version 10.x (OSGi context) as this issue seems to be reproducible in Jetty 10.x (10.0.6) version. |
@trush19 Jetty versions up to Jetty 11 are now at End of Community Support.
You should be using Jetty 12 at this point in time. BTW, Eclipse RT and Eclipse IDE use our artifacts on OSGi without issue. (even back in the Jetty 10.0.6 time period) |
Thank you for the suggestion @joakime . Even after explicitly added these classes using below method still getting classNotFound Exception (java.lang.RuntimeException: java.lang.ClassNotFoundException: org.eclipse.jetty.webapp.WebInfConfiguration) getServerClassMatcher().exclude("org.eclipse.jetty.webapp."); Is there any other way we can include and load these classes? |
Please see this downstream bug we've noticed in Thermostat:
https://bugzilla.redhat.com/show_bug.cgi?id=1353999
The bug lists a reproducer.
Anyway, we are seeing a CNFE for org.eclipse.jetty.webapp.WebInfConfiguration et.al in 9.4.0.M0. Stack trace looks like this
Note the same code works with the 9.3.x train of jetty. The bug might only be happening if jetty is booted via an OSGi framework.
The relevant refactoring which seems to have broken this is here:
a311c8b
The proposed fix would be to revert to the old behavior in WebAppContext.loadConfigurations() to load the config classes with an additional loadClass parameter. To me it looks like the current implementation is incorrect when run in an OSGi container. Using TCCL for loading the config classes is the wrong call.
Thoughts?
The text was updated successfully, but these errors were encountered: