MyFaces Core
  1. MyFaces Core
  2. MYFACES-3051

Use multiple ClassLoaders to find resources (not only ContextClassLoader)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: 2.0.4
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Environment:
      OSGi

      Description

      In most parts of the code we only use the ContextClassLoader to find Classes and Resources. However, in some parts we also use the ClassLoader of the current Class or of a specific Class (e.g. to use myfaces-api and/or myfaces-impl ClassLoader, see ApplicationImpl.getResourceBundle(), BeanValidator.postSetValidationGroups(), ResourceHandlerImpl.getBundle() or FactoryFinder for example).

      IMO we should unify this code and maybe provide a custom ClassLoader that encapsulates three ClassLoaders (ContextClassLoader, myfaces-api and myfaces-impl). This most certainly would solve a lot of OSGi related problems.

      WDYT?

      1. MYFACES-3051-impl-and-shared-2.patch
        41 kB
        Jakob Korherr
      2. MYFACES-3051-impl-and-shared.patch
        36 kB
        Jakob Korherr
      3. MYFACES-3051-first-draft.patch
        8 kB
        Jakob Korherr

        Issue Links

          Activity

          Hide
          Leonardo Uribe added a comment -

          Committed alternate solution to MYFACES-3044

          Show
          Leonardo Uribe added a comment - Committed alternate solution to MYFACES-3044
          Hide
          Leonardo Uribe added a comment -

          JK >> I disagree! What if we want to cache the instance per webapp?

          Why do you disagree? we need concrete and logical reasons to take decisions. I invite you to tell us possible use cases to support this behavior.

          JK >> Really? The best? It is a really easy fix for us

          Remember the code committed does not solve fully the problem.

          JK >> (note that the code is already committed - that's not the patch)

          The fact that the code was committed does not means the code should be let as is and we can't revert it.

          JK >> and you want to tell all OSGi users to provide their custom ResourceHandler for MyFaces' internal resources like jsf.js. I'm sorry, but this is stupid.

          I'm not saying that. It is possible to fix the problem with load jsf.js and oamSubmit.js without use the proposed MyfacesClassloader. I'm objecting the form, not the content.

          It's inevitable OSGi users require to implement some SPI interfaces to make MyFaces work. Why? because OSGi impose a displine about how to manage classloader stuff. To make MyFaces work in OSGi it required to implement the following interfaces:

          • ServiceProviderFinder : To look for META-INF/services/[jsf or myfaces factoryname] files. It is required to override this one, so it can use OSGi code to look on the bundles.
          • AnnotationProvider : To scan annotations
          • FaceletConfigResourceProvider : To look for facelets taglib xml files.
          • FacesConfigResourceProvider : To look for faces-config xml files.
          • FacesConfigurationProvider/FacesConfigurationMerger (optional) : to consolidate all config information.
          • FactoryFinderProvider (soon) : To lookup and create factories.

          The default implementation uses utility class method (ClassLoaderUtils and others), but in some cases like we have on OSGi, those operations are expensive and will not work correctly, so it is necessary to override them. Other thing to consider is how OSGi "deploys" a war. There are different plugins (Spring DM, PAX web extender, Equinox ...) with different config options, and all those ones requires some customization. I invite you to try to setup one environment (not the easy Spring DM because you can see there the TCCL hack, try make PAX work) and you'll have a better idea.

          Really all depends on how you configure OSGi and the plugin you use. But we can't anticipate that, so the best is keep it open, provide the proper SPI stuff so users can do whatever they want.

          JK >> Does that sound acceptable to you?

          Look this class:

          org.apache.myfaces.shared.util.ClassLoaderExtension

          Long time ago, a code was committed to extend the method Class.forName(String name). Look the method on ClassUtils.classForName(). It think it is better to add to ClassLoaderExtensions getResource() and getResourceAsStream(). Note that stuff is used by extscript, so it is well tested and it has a lot of sense to add there (because is a class loader extension, right?).

          Show
          Leonardo Uribe added a comment - JK >> I disagree! What if we want to cache the instance per webapp? Why do you disagree? we need concrete and logical reasons to take decisions. I invite you to tell us possible use cases to support this behavior. JK >> Really? The best? It is a really easy fix for us Remember the code committed does not solve fully the problem. JK >> (note that the code is already committed - that's not the patch) The fact that the code was committed does not means the code should be let as is and we can't revert it. JK >> and you want to tell all OSGi users to provide their custom ResourceHandler for MyFaces' internal resources like jsf.js. I'm sorry, but this is stupid. I'm not saying that. It is possible to fix the problem with load jsf.js and oamSubmit.js without use the proposed MyfacesClassloader. I'm objecting the form, not the content. It's inevitable OSGi users require to implement some SPI interfaces to make MyFaces work. Why? because OSGi impose a displine about how to manage classloader stuff. To make MyFaces work in OSGi it required to implement the following interfaces: ServiceProviderFinder : To look for META-INF/services/ [jsf or myfaces factoryname] files. It is required to override this one, so it can use OSGi code to look on the bundles. AnnotationProvider : To scan annotations FaceletConfigResourceProvider : To look for facelets taglib xml files. FacesConfigResourceProvider : To look for faces-config xml files. FacesConfigurationProvider/FacesConfigurationMerger (optional) : to consolidate all config information. FactoryFinderProvider (soon) : To lookup and create factories. The default implementation uses utility class method (ClassLoaderUtils and others), but in some cases like we have on OSGi, those operations are expensive and will not work correctly, so it is necessary to override them. Other thing to consider is how OSGi "deploys" a war. There are different plugins (Spring DM, PAX web extender, Equinox ...) with different config options, and all those ones requires some customization. I invite you to try to setup one environment (not the easy Spring DM because you can see there the TCCL hack, try make PAX work) and you'll have a better idea. Really all depends on how you configure OSGi and the plugin you use. But we can't anticipate that, so the best is keep it open, provide the proper SPI stuff so users can do whatever they want. JK >> Does that sound acceptable to you? Look this class: org.apache.myfaces.shared.util.ClassLoaderExtension Long time ago, a code was committed to extend the method Class.forName(String name). Look the method on ClassUtils.classForName(). It think it is better to add to ClassLoaderExtensions getResource() and getResourceAsStream(). Note that stuff is used by extscript, so it is well tested and it has a lot of sense to add there (because is a class loader extension, right?).
          Hide
          Jakob Korherr added a comment -

          >If we have a class that we are not expecting to override it or "consume" it in some way, it does not have sense to put it in external context.

          I disagree! What if we want to cache the instance per webapp?

          >Thinking on MYFACES-3044, the best is suggest OSGi users to create a custom ResourceHandler that can locate resources.

          Really? The best? It is a really easy fix for us (note that the code is already committed - that's not the patch) and you want to tell all OSGi users to provide their custom ResourceHandler for MyFaces' internal resources like jsf.js. I'm sorry, but this is stupid. If we do not fix this, I will vote -1 on the next release. Sorry, but that kinda annoys me.

          >From my point of view, we are adding more unnecessary complexity to something that should be simple and clear.

          I disagree. IMHO a custom ClassLoader is a lot easier to use than lots of static methods.

          However that's my point of view against yours, so I'd like to propose a mixed solution for this problem:

          1) use MyFacesClassLoader only inside static methods of ClassLoaderUtils and also in the two ResourceLoaders (fixes MYFACES-3044).
          2) use static methods of ClassLoaderUtils in all other places of the code.

          Does that sound acceptable to you?

          Show
          Jakob Korherr added a comment - >If we have a class that we are not expecting to override it or "consume" it in some way, it does not have sense to put it in external context. I disagree! What if we want to cache the instance per webapp? >Thinking on MYFACES-3044 , the best is suggest OSGi users to create a custom ResourceHandler that can locate resources. Really? The best? It is a really easy fix for us (note that the code is already committed - that's not the patch) and you want to tell all OSGi users to provide their custom ResourceHandler for MyFaces' internal resources like jsf.js. I'm sorry, but this is stupid. If we do not fix this, I will vote -1 on the next release. Sorry, but that kinda annoys me. >From my point of view, we are adding more unnecessary complexity to something that should be simple and clear. I disagree. IMHO a custom ClassLoader is a lot easier to use than lots of static methods. However that's my point of view against yours, so I'd like to propose a mixed solution for this problem: 1) use MyFacesClassLoader only inside static methods of ClassLoaderUtils and also in the two ResourceLoaders (fixes MYFACES-3044 ). 2) use static methods of ClassLoaderUtils in all other places of the code. Does that sound acceptable to you?
          Hide
          Leonardo Uribe added a comment -

          JK >> ClassLoaderUtils basically does the same already, however with a lot of static methods and not with a custom ClassLoader impl (which would be easier IMO).

          That's exactly the reason why it is not necessary such wrapper. In this case, all those methods does not change, it always will be the same.

          JK >> Actually, it will work correctly. Take a closer look at the comments: "use all 3 classloaders and merge WITHOUT duplicates" and "no duplicates".
          JK >> You see, the method uses a Set<URL> in order to automatically remove duplicate URLs.

          Of course, but all that preprocessing is in fact unnecessary, and the ordering of the resources is lost. In this case the order is relevant. Look the javadoc of Clasloader. It says:

          "... The search order is described in the documentation for getResource(String). ..."

          Look the code on Apache ibatis (ClassLoaderWrapper). Note deliberately such class does not extends from Classloader.

          If we have a class that we are not expecting to override it or "consume" it in some way, it does not have sense to put it in external context. At this time, to deal with OSGi we have provided multiple interfaces, so MyFaces can "ask for help" to OSGi to locate resources (faces-config stuff, META-INF/services factory or annotation scanning). Thinking on MYFACES-3044, the best is suggest OSGi users to create a custom ResourceHandler that can locate resources. Really we don't have to do anything. If we do something, we could provide another SPI interface to help our default ResourceHandler implementation to load resource files. That sounds better, because we are in control of what's going on.

          In conclusion, there is not enough justification to do this change. From my point of view, we are adding more unnecessary complexity to something that should be simple and clear. The hack with OSGi (try TCCL first then bundle class loader though someclass.getClassloader()) it is widely used, so really we are not simplifying thing here. Based on this reasoning I have to object this patch.

          Show
          Leonardo Uribe added a comment - JK >> ClassLoaderUtils basically does the same already, however with a lot of static methods and not with a custom ClassLoader impl (which would be easier IMO). That's exactly the reason why it is not necessary such wrapper. In this case, all those methods does not change, it always will be the same. JK >> Actually, it will work correctly. Take a closer look at the comments: "use all 3 classloaders and merge WITHOUT duplicates" and "no duplicates". JK >> You see, the method uses a Set<URL> in order to automatically remove duplicate URLs. Of course, but all that preprocessing is in fact unnecessary, and the ordering of the resources is lost. In this case the order is relevant. Look the javadoc of Clasloader. It says: "... The search order is described in the documentation for getResource(String). ..." Look the code on Apache ibatis (ClassLoaderWrapper). Note deliberately such class does not extends from Classloader. If we have a class that we are not expecting to override it or "consume" it in some way, it does not have sense to put it in external context. At this time, to deal with OSGi we have provided multiple interfaces, so MyFaces can "ask for help" to OSGi to locate resources (faces-config stuff, META-INF/services factory or annotation scanning). Thinking on MYFACES-3044 , the best is suggest OSGi users to create a custom ResourceHandler that can locate resources. Really we don't have to do anything. If we do something, we could provide another SPI interface to help our default ResourceHandler implementation to load resource files. That sounds better, because we are in control of what's going on. In conclusion, there is not enough justification to do this change. From my point of view, we are adding more unnecessary complexity to something that should be simple and clear. The hack with OSGi (try TCCL first then bundle class loader though someclass.getClassloader()) it is widely used, so really we are not simplifying thing here. Based on this reasoning I have to object this patch.
          Hide
          Jakob Korherr added a comment -

          new patch addressing the points mentioned by Leonardo.

          If no objections, I will commit this patch soon.

          Show
          Jakob Korherr added a comment - new patch addressing the points mentioned by Leonardo. If no objections, I will commit this patch soon.
          Hide
          Jakob Korherr added a comment -

          OK, I see your point, Leo, but I think the general idea of a MyFacesClassLoader is good enough to follow this approach. It comes with the advantage that we do not have to think about which ClassLoader may be the right one in different scenarios (OSGi vs. non-OSGi, MyFaces resources vs. webapp resource). ClassLoaderUtils basically does the same already, however with a lot of static methods and not with a custom ClassLoader impl (which would be easier IMO).

          Fortunately, I think the problems you're seeing can be fixed easily:

          >It is ok, but note if we are using myfaces-bundle, we'll scan on the same classloader twice (because api and impl are the same bundle classloader)

          The solution is to change MyFacesClassLoader to check if api and impl ClassLoader are actually the same and only use one in this case.

          >This code will not work correctly:
          >
          > @Override
          > public Enumeration<URL> getResources(String s) throws IOException
          > {
          > // use all 3 classloaders and merge without duplicates
          > Set<URL> urls = new HashSet<URL>(); // no duplicates
          >
          > // context classloader
          > urls.addAll(Collections.list(super.getResources(s)));
          >
          > // api classlaoder
          > urls.addAll(Collections.list(apiClassLoader.getResources(s)));
          >
          > // impl classlaoder
          > urls.addAll(Collections.list(implClassLoader.getResources(s)));
          >
          > return Collections.enumeration(urls);

          Actually, it will work correctly. Take a closer look at the comments: "use all 3 classloaders and merge WITHOUT duplicates" and "no duplicates". You see, the method uses a Set<URL> in order to automatically remove duplicate URLs.

          In addition, MYFACES-3044 was solved by the MyFacesClassLoader already. So why should we remove it?

          I'll provide a new patch to address the points mentioned.

          Show
          Jakob Korherr added a comment - OK, I see your point, Leo, but I think the general idea of a MyFacesClassLoader is good enough to follow this approach. It comes with the advantage that we do not have to think about which ClassLoader may be the right one in different scenarios (OSGi vs. non-OSGi, MyFaces resources vs. webapp resource). ClassLoaderUtils basically does the same already, however with a lot of static methods and not with a custom ClassLoader impl (which would be easier IMO). Fortunately, I think the problems you're seeing can be fixed easily: >It is ok, but note if we are using myfaces-bundle, we'll scan on the same classloader twice (because api and impl are the same bundle classloader) The solution is to change MyFacesClassLoader to check if api and impl ClassLoader are actually the same and only use one in this case. >This code will not work correctly: > > @Override > public Enumeration<URL> getResources(String s) throws IOException > { > // use all 3 classloaders and merge without duplicates > Set<URL> urls = new HashSet<URL>(); // no duplicates > > // context classloader > urls.addAll(Collections.list(super.getResources(s))); > > // api classlaoder > urls.addAll(Collections.list(apiClassLoader.getResources(s))); > > // impl classlaoder > urls.addAll(Collections.list(implClassLoader.getResources(s))); > > return Collections.enumeration(urls); Actually, it will work correctly. Take a closer look at the comments: "use all 3 classloaders and merge WITHOUT duplicates" and "no duplicates". You see, the method uses a Set<URL> in order to automatically remove duplicate URLs. In addition, MYFACES-3044 was solved by the MyFacesClassLoader already. So why should we remove it? I'll provide a new patch to address the points mentioned.
          Hide
          Leonardo Uribe added a comment -

          Unfortunately, the patch has some problems. I see some code already committed, but fortunately it does not contains too much changes.

          Let's start with the idea of MyFacesClassLoader. Look this code:

          @Override
          public URL getResource(String s)
          {
          // context classloader
          URL url = super.getResource(s);

          if (url == null)
          {
          // try api
          url = apiClassLoader.getResource(s);

          if (url == null)

          { // try impl url = implClassLoader.getResource(s); }

          It is ok, but note if we are using myfaces-bundle, we'll scan on the same classloader twice (because api and impl are the same bundle classloader)

          This code will not work correctly:

          @Override
          public Enumeration<URL> getResources(String s) throws IOException
          {
          // use all 3 classloaders and merge without duplicates
          Set<URL> urls = new HashSet<URL>(); // no duplicates

          // context classloader
          urls.addAll(Collections.list(super.getResources(s)));

          // api classlaoder
          urls.addAll(Collections.list(apiClassLoader.getResources(s)));

          // impl classlaoder
          urls.addAll(Collections.list(implClassLoader.getResources(s)));

          return Collections.enumeration(urls);

          Why? the class loader IS NOT unique per class. Really it depends on the environment. In some scenarios, the same classloader is returned from different classes. In OSGi case, since it is expected myfaces-bundle is used we are adding the same resources 2 times, and if the TCCL can locate resources of myfaces bundle jar, the final result is have 3 times the same files.

          Look this issue: MYFACES-3055 META-INF/faces-config.xml files in JARs are loaded twice . The problem is caused because somebody decide to do the same the patch here is proposing.

          Unfortunately, by the previous reason it is not possible to use a MyFacesClassLoader. The best we can do here is let the code as we had before, so we can differentiate between use TCCL and class ClassLoader. Instead, it is better to have utility methods that handle the case we require (retrieve resource from TCCL and myfaces-bundle CL). There is no need to worry about myfaces-api/myfaces-impl, because that's one reason to have just one jar file in OSGi case (myfaces-bundle).

          Really there is one problem about MyFaces and OSGi. Sometimes, MyFaces requires to instantiate/load resources or classes inside/outside MyFaces. For example, I have one JSF component library that has some resources under META-INF/resources/my.custom.library and MyFaces requires to serve this resource on a page. First MyFaces should scans on its bundle classloader and it does not found the files required. Then, it uses the TCCL to locate the files, and that one "contacts" the custom JSF component library jar to see if this bundle has the files and if that so it serves the required resources.

          Right now, MyFaces REQUIRES the TCCL to work as expected. That was one lesson learned trying to close MYFACES-2290. The problem in that time was FactoryFinder imposes that restriction. Note right now, it is probably we'll commit soon a solution to override FactoryFinder behavior. But the problem we need to think is if there is other way in OSGi to make MyFaces load some resource files in a web context or app context. For now, it is supposed the TCCL do the job (and it does fine), but we don't have any report indicating it is necessary to suppose the opposite.

          Show
          Leonardo Uribe added a comment - Unfortunately, the patch has some problems. I see some code already committed, but fortunately it does not contains too much changes. Let's start with the idea of MyFacesClassLoader. Look this code: @Override public URL getResource(String s) { // context classloader URL url = super.getResource(s); if (url == null) { // try api url = apiClassLoader.getResource(s); if (url == null) { // try impl url = implClassLoader.getResource(s); } It is ok, but note if we are using myfaces-bundle, we'll scan on the same classloader twice (because api and impl are the same bundle classloader) This code will not work correctly: @Override public Enumeration<URL> getResources(String s) throws IOException { // use all 3 classloaders and merge without duplicates Set<URL> urls = new HashSet<URL>(); // no duplicates // context classloader urls.addAll(Collections.list(super.getResources(s))); // api classlaoder urls.addAll(Collections.list(apiClassLoader.getResources(s))); // impl classlaoder urls.addAll(Collections.list(implClassLoader.getResources(s))); return Collections.enumeration(urls); Why? the class loader IS NOT unique per class. Really it depends on the environment. In some scenarios, the same classloader is returned from different classes. In OSGi case, since it is expected myfaces-bundle is used we are adding the same resources 2 times, and if the TCCL can locate resources of myfaces bundle jar, the final result is have 3 times the same files. Look this issue: MYFACES-3055 META-INF/faces-config.xml files in JARs are loaded twice . The problem is caused because somebody decide to do the same the patch here is proposing. Unfortunately, by the previous reason it is not possible to use a MyFacesClassLoader. The best we can do here is let the code as we had before, so we can differentiate between use TCCL and class ClassLoader. Instead, it is better to have utility methods that handle the case we require (retrieve resource from TCCL and myfaces-bundle CL). There is no need to worry about myfaces-api/myfaces-impl, because that's one reason to have just one jar file in OSGi case (myfaces-bundle). Really there is one problem about MyFaces and OSGi. Sometimes, MyFaces requires to instantiate/load resources or classes inside/outside MyFaces. For example, I have one JSF component library that has some resources under META-INF/resources/my.custom.library and MyFaces requires to serve this resource on a page. First MyFaces should scans on its bundle classloader and it does not found the files required. Then, it uses the TCCL to locate the files, and that one "contacts" the custom JSF component library jar to see if this bundle has the files and if that so it serves the required resources. Right now, MyFaces REQUIRES the TCCL to work as expected. That was one lesson learned trying to close MYFACES-2290 . The problem in that time was FactoryFinder imposes that restriction. Note right now, it is probably we'll commit soon a solution to override FactoryFinder behavior. But the problem we need to think is if there is other way in OSGi to make MyFaces load some resource files in a web context or app context. For now, it is supposed the TCCL do the job (and it does fine), but we don't have any report indicating it is necessary to suppose the opposite.
          Hide
          Jakob Korherr added a comment -

          If no objections, I will commit MYFACES-3051-impl-and-shared.patch soon!

          Show
          Jakob Korherr added a comment - If no objections, I will commit MYFACES-3051 -impl-and-shared.patch soon!
          Hide
          Jakob Korherr added a comment -

          MYFACES-3051-impl-and-shared.patch replaces the ContextClassLoader in the shared and impl modules with the MyFacesClassLoader.

          Only if the ContextClassLoader is used to identify the current web application, it does not replace it (you need this in shared classloader scenarios).

          Show
          Jakob Korherr added a comment - MYFACES-3051 -impl-and-shared.patch replaces the ContextClassLoader in the shared and impl modules with the MyFacesClassLoader. Only if the ContextClassLoader is used to identify the current web application, it does not replace it (you need this in shared classloader scenarios).
          Hide
          Jakob Korherr added a comment -

          patch of first draft for this issue that should solve MYFACES-3044

          Show
          Jakob Korherr added a comment - patch of first draft for this issue that should solve MYFACES-3044
          Show
          Jakob Korherr added a comment - Just found something similar at Apache ibatis: http://svn.apache.org/repos/asf/ibatis/java/ibatis-3/tags/java_release_3.0.0-196_beta_2/ibatis-3-core/src/main/java/org/apache/ibatis/io/ClassLoaderWrapper.java
          Hide
          Jakob Korherr added a comment -

          This method from ResourceHandlerImpl is a perfect example for the ClassLoader mechansim I am describing:

          private static ResourceBundle getBundle(FacesContext facesContext, Locale locale, String bundleName)
          {
          try

          { // First we try the JSF implementation class loader return ResourceBundle.getBundle(bundleName, locale, ResourceHandlerImpl.class.getClassLoader()); }

          catch (MissingResourceException ignore1)
          {
          try

          { // Next we try the JSF API class loader return ResourceBundle.getBundle(bundleName, locale, ResourceHandler.class.getClassLoader()); }

          catch (MissingResourceException ignore2)
          {
          try

          { // Last resort is the context class loader return ResourceBundle.getBundle(bundleName, locale, ClassUtils.getContextClassLoader()); }

          catch (MissingResourceException damned)

          { return null; }

          }
          }
          }

          Show
          Jakob Korherr added a comment - This method from ResourceHandlerImpl is a perfect example for the ClassLoader mechansim I am describing: private static ResourceBundle getBundle(FacesContext facesContext, Locale locale, String bundleName) { try { // First we try the JSF implementation class loader return ResourceBundle.getBundle(bundleName, locale, ResourceHandlerImpl.class.getClassLoader()); } catch (MissingResourceException ignore1) { try { // Next we try the JSF API class loader return ResourceBundle.getBundle(bundleName, locale, ResourceHandler.class.getClassLoader()); } catch (MissingResourceException ignore2) { try { // Last resort is the context class loader return ResourceBundle.getBundle(bundleName, locale, ClassUtils.getContextClassLoader()); } catch (MissingResourceException damned) { return null; } } } }

            People

            • Assignee:
              Jakob Korherr
              Reporter:
              Jakob Korherr
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development