Lucene - Core
  1. Lucene - Core
  2. LUCENE-4713

SPI: Allow fallback to default ClassLoader if Thread#getContextClassLoader fails

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 4.0, 4.1, 4.2
    • Fix Version/s: 4.3, 4.2.1, 6.0
    • Component/s: None
    • Lucene Fields:
      New, Patch Available

      Description

      NOTE: This issue has been renamed from:
      "Replace calls to Thread#getContextClassLoader with the ClassLoader of the current class"
      because the revised patch provides a clean fallback path.

      I am not sure whether it is a design decision or if we can indeed consider this a bug:

      In core and analysis-common some classes provide on-demand class loading using SPI. In NamedSPILoader, SPIClassIterator, ClasspathResourceLoader and AnalysisSPILoader there are constructors that use the Thread's context ClassLoader by default whenever no particular other ClassLoader was specified.

      Unfortunately this does not work as expected when the Thread's ClassLoader can't see the required classes that are instantiated downstream with the help of Class.forName (e.g., Codecs, Analyzers, etc.).

      That's what happened to us here. We currently experiment with running Lucene 2.9 and 4.x in one JVM, both being separated by custom ClassLoaders, each seeing only the corresponding Lucene version and the upstream classpath.

      While NamedSPILoader and company get successfully loaded by our custom ClassLoader, their instantiation fails because our Thread's Context-ClassLoader cannot find the additionally required classes.

      We could probably work-around this by using Thread#setContextClassLoader at construction time (and quickly reverting back afterwards), but I have the impression this might just hide the actual problem and cause further trouble when lazy-loading classes later on, and potentially from another Thread.

      Removing the call to Thread#getContextClassLoader would also align with the behavior of AttributeSource.DEFAULT_ATTRIBUTE_FACTORY, which in fact uses Attribute#getClass().getClassLoader() instead.

      A simple patch is attached. All tests pass.

      1. LUCENE-4713.patch
        3 kB
        Uwe Schindler
      2. LUCENE-4713.patch
        3 kB
        Uwe Schindler
      3. LUCENE-4713.patch
        3 kB
        Christian Kohlschütter
      4. LUCENE-4713.patch
        3 kB
        Christian Kohlschütter
      5. LUCENE-4713.patch
        3 kB
        Uwe Schindler
      6. LUCENE-4713.patch
        1 kB
        Uwe Schindler
      7. LUCENE-4713.patch
        19 kB
        Christian Kohlschütter
      8. LuceneContextClassLoader.patch
        3 kB
        Christian Kohlschütter

        Activity

        Hide
        Dawid Weiss added a comment -

        I bet there are people who need the exact opposite (context class loader lookup as opposed to defining class's class loader lookup). If you perform classloader isolation I think you'll have to switch the context classloader accordingly, much like servlet containers to it when handling requests? Just my opinion, I'm not saying this is a wrong idea.

        Show
        Dawid Weiss added a comment - I bet there are people who need the exact opposite (context class loader lookup as opposed to defining class's class loader lookup). If you perform classloader isolation I think you'll have to switch the context classloader accordingly, much like servlet containers to it when handling requests? Just my opinion, I'm not saying this is a wrong idea.
        Hide
        Uwe Schindler added a comment -

        I disagree about the issue. The Java specification of ServiceLoader mandates that the context class loader is used by default: http://docs.oracle.com/javase/6/docs/api/java/util/ServiceLoader.html#load(java.lang.Class) - We are doing nothing different here, we just have a own implementation of ServiceLoader, because the Java 6 one has some bugs and inconsistencies on several JVM vendors.

        In fact, if you want classloader isolation, you should also separate threads and give every thread the corresponding isolated classloader, like Dawid said (Tomcat and Jetty are doing this, too).

        Show
        Uwe Schindler added a comment - I disagree about the issue. The Java specification of ServiceLoader mandates that the context class loader is used by default: http://docs.oracle.com/javase/6/docs/api/java/util/ServiceLoader.html#load(java.lang.Class) - We are doing nothing different here, we just have a own implementation of ServiceLoader, because the Java 6 one has some bugs and inconsistencies on several JVM vendors. In fact, if you want classloader isolation, you should also separate threads and give every thread the corresponding isolated classloader, like Dawid said (Tomcat and Jetty are doing this, too).
        Hide
        Christian Kohlschütter added a comment -

        Good point, Uwe.

        But maintaining separated, ClassLoader-specific Threads seems like real overkill here.
        Couldn't we at least fall-back on the default ClassLoader if the Thread's Context-ClassLoader fails to load the class/resource?

        This way, it will work for both cases.

        What do you think?

        Show
        Christian Kohlschütter added a comment - Good point, Uwe. But maintaining separated, ClassLoader-specific Threads seems like real overkill here. Couldn't we at least fall-back on the default ClassLoader if the Thread's Context-ClassLoader fails to load the class/resource? This way, it will work for both cases. What do you think?
        Hide
        Uwe Schindler added a comment -

        I don't think you have a chance to not set the correct context classloader... Lucene is not the only this that relies on this (XML parser SPIs, java.security, imaging file formats, Charsets, ...)

        Using more than one class loader to lookup SPIs is possible but violates the spec. It also slows down, because you have to do several lookups, filter duplicates and so on.

        One thing you can do is: Codec.reloadCodescs(yourClassloader), PostingFormat.reloadPostingsFormats(yourClassLoader) before using Lucene (and the same for the other SPIs in Lucene). This will rescan the given classloader and add new, additional codecs found to the internal list. Solr is doing the same after initializing the SolrResourceLoader (to allow codecs be loaded from the Solr plugins folder).

        I already have the plans to add one Util class that does this for all SPIs, Lucene uses (currently you have to do this separately for all of them).

        Show
        Uwe Schindler added a comment - I don't think you have a chance to not set the correct context classloader... Lucene is not the only this that relies on this (XML parser SPIs, java.security, imaging file formats, Charsets, ...) Using more than one class loader to lookup SPIs is possible but violates the spec. It also slows down, because you have to do several lookups, filter duplicates and so on. One thing you can do is: Codec.reloadCodescs(yourClassloader), PostingFormat.reloadPostingsFormats(yourClassLoader) before using Lucene (and the same for the other SPIs in Lucene). This will rescan the given classloader and add new, additional codecs found to the internal list. Solr is doing the same after initializing the SolrResourceLoader (to allow codecs be loaded from the Solr plugins folder). I already have the plans to add one Util class that does this for all SPIs, Lucene uses (currently you have to do this separately for all of them).
        Hide
        Christian Kohlschütter added a comment - - edited

        Revised patch.

        Now uses Thread#getContextClassLoader and falls back to the default ClassLoader on ClassNotFoundException/missing resource only, with the help of a lightweight FallbackClassLoader.
        This should not incur any slowdowns, does not require filtering, etc.

        Regarding your suggestion concerning Codecs.reload, etc.: I would definitely not want to have these rather-internal calls in the application code. So providing an Util class that hides the SPI details would be essential.

        Practically, what you describe may boil down to a static Lucene bootstrap method, e.g. a "Lucene.init()". I am not really sure if this is a good idea, especially if the only benefit is that we abide by the SPI spec to the very last word.

        Show
        Christian Kohlschütter added a comment - - edited Revised patch. Now uses Thread#getContextClassLoader and falls back to the default ClassLoader on ClassNotFoundException/missing resource only, with the help of a lightweight FallbackClassLoader. This should not incur any slowdowns, does not require filtering, etc. Regarding your suggestion concerning Codecs.reload, etc.: I would definitely not want to have these rather-internal calls in the application code. So providing an Util class that hides the SPI details would be essential. Practically, what you describe may boil down to a static Lucene bootstrap method, e.g. a "Lucene.init()". I am not really sure if this is a good idea, especially if the only benefit is that we abide by the SPI spec to the very last word.
        Hide
        Christian Kohlschütter added a comment -

        Uwe,

        I have tried your suggested workaround. Unfortunately it does not work.

        The Codec class has a static field that already requires a working SPI setup:

        private static Codec defaultCodec = Codec.forName("Lucene41");

        Calling Codec.reloadCodecs with my ClassLoader will use the Thread-context ClassLoader (via Codec#loader) and thus fails with

        java.lang.IllegalArgumentException: A SPI class of type org.apache.lucene.codecs.Codec with name 'Lucene40' does not exist. You need to add the corresponding JAR file supporting this SPI to your classpath.The current classpath supports the following names: []
        at org.apache.lucene.util.NamedSPILoader.lookup(NamedSPILoader.java:104)
        at org.apache.lucene.codecs.Codec.forName(Codec.java:95)
        at org.apache.lucene.codecs.Codec.<clinit>(Codec.java:122)
        ... 28 more

        (stack trace from Lucene 4.0, still valid for 4.1).

        I could resolve this by moving Codec.forName into Codec#getDefault:

        private static Codec defaultCodec = null;
          
          /** expert: returns the default codec used for newly created
           *  {@link IndexWriterConfig}s.
           */
          // TODO: should we use this, or maybe a system property is better?
          public static Codec getDefault() {
            if(defaultCodec == null) {
              defaultCodec = Codec.forName("Lucene40");      
            }
            return defaultCodec;
          }
        

        Moreover, it is important to call PostingsFormat#reloadPostingsFormats before Codec#reloadCodecs, because the Lucene40 codec looks up the PostingFormat class...

        As you can see, fixing this easily becomes a mess – we would have to find a specific order of what to load first. Finally, in the case of AnalysisSPILoader there is no static method to call...

        I am pretty sure that it is safer to use the fall-back ClassLoader I provided in the patch.

        Show
        Christian Kohlschütter added a comment - Uwe, I have tried your suggested workaround. Unfortunately it does not work. The Codec class has a static field that already requires a working SPI setup: private static Codec defaultCodec = Codec.forName("Lucene41"); Calling Codec.reloadCodecs with my ClassLoader will use the Thread-context ClassLoader (via Codec#loader) and thus fails with java.lang.IllegalArgumentException: A SPI class of type org.apache.lucene.codecs.Codec with name 'Lucene40' does not exist. You need to add the corresponding JAR file supporting this SPI to your classpath.The current classpath supports the following names: [] at org.apache.lucene.util.NamedSPILoader.lookup(NamedSPILoader.java:104) at org.apache.lucene.codecs.Codec.forName(Codec.java:95) at org.apache.lucene.codecs.Codec.<clinit>(Codec.java:122) ... 28 more (stack trace from Lucene 4.0, still valid for 4.1). I could resolve this by moving Codec.forName into Codec#getDefault: private static Codec defaultCodec = null; /** expert: returns the default codec used for newly created * {@link IndexWriterConfig}s. */ // TODO: should we use this, or maybe a system property is better? public static Codec getDefault() { if(defaultCodec == null) { defaultCodec = Codec.forName("Lucene40"); } return defaultCodec; } Moreover, it is important to call PostingsFormat#reloadPostingsFormats before Codec#reloadCodecs, because the Lucene40 codec looks up the PostingFormat class... As you can see, fixing this easily becomes a mess – we would have to find a specific order of what to load first. Finally, in the case of AnalysisSPILoader there is no static method to call... I am pretty sure that it is safer to use the fall-back ClassLoader I provided in the patch.
        Hide
        Vojtěch Toman added a comment -

        We hit the same issue when using Lucene via Ant. After upgrading from Lucene 3.x to 4.0, our Lucene-related Ant tasks stopped working because the class loader returned by Thread.currentThread().getContextClassLoader() does not see META-INF/services/org.apache.lucene.codecs.Codec.

        java.lang.ExceptionInInitializerError
                at org.apache.lucene.index.LiveIndexWriterConfig.<init>(LiveIndexWriterConfig.java:118)
                at org.apache.lucene.index.IndexWriterConfig.<init>(IndexWriterConfig.java:145)
                [.....left out.....]
                at com.xhive.anttasks.XhiveTask.execute(XhiveTask.java:180)
                at org.apache.tools.ant.UnknownElement.execute(UnknownElement.java:291)
                at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
                at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
                at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
                at java.lang.reflect.Method.invoke(Method.java:597)
                at org.apache.tools.ant.dispatch.DispatchUtils.execute(DispatchUtils.java:106)
                at org.apache.tools.ant.Task.perform(Task.java:348)
                at org.apache.tools.ant.Target.execute(Target.java:390)
                at org.apache.tools.ant.Target.performTasks(Target.java:411)
                at org.apache.tools.ant.Project.executeSortedTargets(Project.java:1399)
                at org.apache.tools.ant.Project.executeTarget(Project.java:1368)
                at org.apache.tools.ant.helper.DefaultExecutor.executeTargets(DefaultExecutor.java:41)
                at org.apache.tools.ant.Project.executeTargets(Project.java:1251)
                at org.apache.tools.ant.Main.runBuild(Main.java:809)
                at org.apache.tools.ant.Main.startAnt(Main.java:217)
                at org.apache.tools.ant.launch.Launcher.run(Launcher.java:280)
                at org.apache.tools.ant.launch.Launcher.main(Launcher.java:109)
        Caused by: java.lang.IllegalArgumentException: A SPI class of type org.apache.lucene.codecs.Codec with name 'Lucene40' does not exist. You need to add the corresponding JAR file supporting this SPI to your classpath.The current classpath supports the following names: []
                at org.apache.lucene.util.NamedSPILoader.lookup(NamedSPILoader.java:104)
                at org.apache.lucene.codecs.Codec.forName(Codec.java:95)
                at org.apache.lucene.codecs.Codec.<clinit>(Codec.java:122)
                ... 34 more
        

        The build file looks like this:

        <project>
        
          ...
        
          <path id="classpath">
            <fileset dir="${xhive.dir}">
               <!-- contains, among others, lucene.jar -->
              <include name="lib/xhivedb/**/*.jar"/>
              <!-- contains, among others, our custom Ant tasks -->
              <include name="build/jarfiles/plain/xhive.jar"/>
            </fileset>
          </path>
        
          ...
        
          <target name="build">
            <taskdef classpathref="classpath"
                     resource="com/xhive/anttasks/tasks.properties"/>
        
            <!-- this custom task fails after migration to Lucene 4.x -->
            <our-lucene-related-task .../>
          </target>
        
        </project>
        

        What happens is that inside the task Thread.currentThread().getContextClassLoader() does not return the Ant class loader (which knows the full classpath).

        Doing something like this:

        public NamedSPILoader(Class<S> clazz) {
          this(clazz, clazz.getClassLoader());
        }
        

        would fix it, but given the previous comments on the issue, I am not sure it would be the best fix.

        Show
        Vojtěch Toman added a comment - We hit the same issue when using Lucene via Ant. After upgrading from Lucene 3.x to 4.0, our Lucene-related Ant tasks stopped working because the class loader returned by Thread.currentThread().getContextClassLoader() does not see META-INF/services/org.apache.lucene.codecs.Codec. java.lang.ExceptionInInitializerError at org.apache.lucene.index.LiveIndexWriterConfig.<init>(LiveIndexWriterConfig.java:118) at org.apache.lucene.index.IndexWriterConfig.<init>(IndexWriterConfig.java:145) [.....left out.....] at com.xhive.anttasks.XhiveTask.execute(XhiveTask.java:180) at org.apache.tools.ant.UnknownElement.execute(UnknownElement.java:291) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at org.apache.tools.ant.dispatch.DispatchUtils.execute(DispatchUtils.java:106) at org.apache.tools.ant.Task.perform(Task.java:348) at org.apache.tools.ant.Target.execute(Target.java:390) at org.apache.tools.ant.Target.performTasks(Target.java:411) at org.apache.tools.ant.Project.executeSortedTargets(Project.java:1399) at org.apache.tools.ant.Project.executeTarget(Project.java:1368) at org.apache.tools.ant.helper.DefaultExecutor.executeTargets(DefaultExecutor.java:41) at org.apache.tools.ant.Project.executeTargets(Project.java:1251) at org.apache.tools.ant.Main.runBuild(Main.java:809) at org.apache.tools.ant.Main.startAnt(Main.java:217) at org.apache.tools.ant.launch.Launcher.run(Launcher.java:280) at org.apache.tools.ant.launch.Launcher.main(Launcher.java:109) Caused by: java.lang.IllegalArgumentException: A SPI class of type org.apache.lucene.codecs.Codec with name 'Lucene40' does not exist. You need to add the corresponding JAR file supporting this SPI to your classpath.The current classpath supports the following names: [] at org.apache.lucene.util.NamedSPILoader.lookup(NamedSPILoader.java:104) at org.apache.lucene.codecs.Codec.forName(Codec.java:95) at org.apache.lucene.codecs.Codec.<clinit>(Codec.java:122) ... 34 more The build file looks like this: <project> ... <path id= "classpath" > <fileset dir= "${xhive.dir}" > <!-- contains, among others, lucene.jar --> <include name= "lib/xhivedb/**/*.jar" /> <!-- contains, among others, our custom Ant tasks --> <include name= "build/jarfiles/plain/xhive.jar" /> </fileset> </path> ... <target name= "build" > <taskdef classpathref= "classpath" resource= "com/xhive/anttasks/tasks.properties" /> <!-- this custom task fails after migration to Lucene 4.x --> <our-lucene-related-task .../> </target> </project> What happens is that inside the task Thread.currentThread().getContextClassLoader() does not return the Ant class loader (which knows the full classpath). Doing something like this: public NamedSPILoader( Class <S> clazz) { this (clazz, clazz.getClassLoader()); } would fix it, but given the previous comments on the issue, I am not sure it would be the best fix.
        Hide
        Uwe Schindler added a comment -

        Use Codecs.reloadCodecs(antClassLoader) in your application initialization code. The same method exists for PostingsFormats, Unfortunately there is no method to automatically reload all SPIs in Lucene.

        Show
        Uwe Schindler added a comment - Use Codecs.reloadCodecs(antClassLoader) in your application initialization code. The same method exists for PostingsFormats, Unfortunately there is no method to automatically reload all SPIs in Lucene.
        Hide
        Uwe Schindler added a comment -

        From my persepctive, Christian Kohlschütters suggestion is nice to have. We should at least enforce that the classloader that loaded the lucene-core.jar file is also scanned, regardless what the context class loader is - this would somehow "emulate" what the JDK does wth its own extensions like XML parsers. In any case, we would need to decide, what to do first (the Lucene class loader or the context one).

        I will provide a patch.

        Show
        Uwe Schindler added a comment - From my persepctive, Christian Kohlschütters suggestion is nice to have. We should at least enforce that the classloader that loaded the lucene-core.jar file is also scanned, regardless what the context class loader is - this would somehow "emulate" what the JDK does wth its own extensions like XML parsers. In any case, we would need to decide, what to do first (the Lucene class loader or the context one). I will provide a patch.
        Hide
        Uwe Schindler added a comment - - edited

        This is the easiest patch possible. Still lacks some documentation (to actually document that the Lucene class loader is scanned), but ensures that at least all SPIs shipped with Lucene are visible.

        If a user has additional SPIs outside Lucene core, then its his turn to make them correctly available.

        The Lucene classloader is scanned before the context one, because the classes shipped with lucene should take precedence. On the other hand, this makes it impossible to "override" Lucene's default codec unless you place the jar file next to lucene-core.jar in same classloader.

        Show
        Uwe Schindler added a comment - - edited This is the easiest patch possible. Still lacks some documentation (to actually document that the Lucene class loader is scanned), but ensures that at least all SPIs shipped with Lucene are visible. If a user has additional SPIs outside Lucene core, then its his turn to make them correctly available. The Lucene classloader is scanned before the context one, because the classes shipped with lucene should take precedence. On the other hand, this makes it impossible to "override" Lucene's default codec unless you place the jar file next to lucene-core.jar in same classloader.
        Hide
        Christian Kohlschütter added a comment -

        Thanks, Uwe! Looks good and works well in our setup.

        Regarding overriding Lucene's default codec implementations:
        We anyways have to place any other modified, non-SPI Lucene classes in the same ClassLoader, so I really appreciate that this patch enforces this.

        Show
        Christian Kohlschütter added a comment - Thanks, Uwe! Looks good and works well in our setup. Regarding overriding Lucene's default codec implementations: We anyways have to place any other modified, non-SPI Lucene classes in the same ClassLoader, so I really appreciate that this patch enforces this.
        Hide
        Uwe Schindler added a comment - - edited

        Hi Christian,
        another patch, with some optimization. The clazz's classloader is only scanned, if its not a parent or the same. If the Lucene's clazz' classloader is a parent of the context one, it does not need to scan it.
        This also works around the problems with "hiding" classes. To override the Lucene core codecs, e.g. Tomcat's classloader (J2EE) will use parent-last semantics, and in that case the precedence goes to the webapp.
        Only if the lucene classloader is not at all related to the context one, it is scanned.

        Can you try this, too? Unfortunately its hard to write a good testcase without some fake classes in separate compilation units which complicates the Lucene build

        Show
        Uwe Schindler added a comment - - edited Hi Christian, another patch, with some optimization. The clazz's classloader is only scanned, if its not a parent or the same. If the Lucene's clazz' classloader is a parent of the context one, it does not need to scan it. This also works around the problems with "hiding" classes. To override the Lucene core codecs, e.g. Tomcat's classloader (J2EE) will use parent-last semantics, and in that case the precedence goes to the webapp. Only if the lucene classloader is not at all related to the context one, it is scanned. Can you try this, too? Unfortunately its hard to write a good testcase without some fake classes in separate compilation units which complicates the Lucene build
        Hide
        Uwe Schindler added a comment -

        Regarding overriding Lucene's default codec implementations: We anyways have to place any other modified, non-SPI Lucene classes in the same ClassLoader, so I really appreciate that this patch enforces this.

        Overriding default Lucene Codecs doesn't need to necessarily use the same class name. Codecs are identified by their "name" as written into the index files (e.g., "Lucene42"). If you implement another subclass of Codec with the same name, but different class name, it is also taken into account. But in any case, the class file must be listed before the lucene-core.jar one in classpath (btw, this is used in Lucene 4.x, to allow a READ/WRITE variant of the Lucene3x codec for testing only. The test-framework.jar simply exposes another class, extending the original READONLY Lucene3x codec to support WRITE, but makeing it available also with the "Lucene3x" name to the loader).

        Show
        Uwe Schindler added a comment - Regarding overriding Lucene's default codec implementations: We anyways have to place any other modified, non-SPI Lucene classes in the same ClassLoader, so I really appreciate that this patch enforces this. Overriding default Lucene Codecs doesn't need to necessarily use the same class name. Codecs are identified by their "name" as written into the index files (e.g., "Lucene42"). If you implement another subclass of Codec with the same name, but different class name, it is also taken into account. But in any case, the class file must be listed before the lucene-core.jar one in classpath (btw, this is used in Lucene 4.x, to allow a READ/WRITE variant of the Lucene3x codec for testing only. The test-framework.jar simply exposes another class, extending the original READONLY Lucene3x codec to support WRITE, but makeing it available also with the "Lucene3x" name to the loader).
        Hide
        Christian Kohlschütter added a comment -

        Works for me, too. Those corner cases...

        One thing that I stumbled upon was that Thread#getContextClassLoader may actually return null.
        We currently throw an IllegalArgumentException in this case, which can be considered a bug by itself.

        If we decide that a fix for this bug is to check for null and use the classes' default ClassLoader instead, we would actually call #reload twice (because isParentClassLoader will return false if child==null).

        See the attached patch for a proposed fix.

        Show
        Christian Kohlschütter added a comment - Works for me, too. Those corner cases... One thing that I stumbled upon was that Thread#getContextClassLoader may actually return null. We currently throw an IllegalArgumentException in this case, which can be considered a bug by itself. If we decide that a fix for this bug is to check for null and use the classes' default ClassLoader instead, we would actually call #reload twice (because isParentClassLoader will return false if child==null). See the attached patch for a proposed fix.
        Hide
        Uwe Schindler added a comment - - edited

        There is another problem: The abstract clazz' classloader may be null, too (although this never happens in recent JDKs): The bootstrap class loader may be null. But we don't have the problem here, as Lucene classes are never ever loaded through the boot class loader (but e.g. String.class.getClassLoader() may return null).

        I dont like hooking also into reload(), I will think of another more elegant solution). But to mention: If the context class loader is null (which cannot happen unless you explicitly set it to null), Java's own classloading for SPIs would be broken, too (see the implementation of java.util.ServiceLoader). (EDIT: Java's ServiceLoader uses SystemClassLoader if context loader is null)

        Show
        Uwe Schindler added a comment - - edited There is another problem: The abstract clazz' classloader may be null, too (although this never happens in recent JDKs): The bootstrap class loader may be null. But we don't have the problem here, as Lucene classes are never ever loaded through the boot class loader (but e.g. String.class.getClassLoader() may return null). I dont like hooking also into reload(), I will think of another more elegant solution). But to mention: If the context class loader is null (which cannot happen unless you explicitly set it to null), Java's own classloading for SPIs would be broken, too (see the implementation of java.util.ServiceLoader). (EDIT: Java's ServiceLoader uses SystemClassLoader if context loader is null)
        Hide
        Christian Kohlschütter added a comment -

        This patch keeps #reload untouched.

        Show
        Christian Kohlschütter added a comment - This patch keeps #reload untouched.
        Hide
        Uwe Schindler added a comment -

        Here is the patch that mimics what the "original" java.util.ServiceLoader does:
        If the classloader (e.g. the context classloader) is null, it uses the system classloader. The exception on null classloader was removed.

        The patch then also adds some null checks, so the "fallback" case is only used if both possible loaders are != null.
        If all class loaders are null, the system loader is used, which should never happen, as Lucene is not part of rt.jar.

        I think this is ready. Unfortunately we had some overlap, Christian

        Show
        Uwe Schindler added a comment - Here is the patch that mimics what the "original" java.util.ServiceLoader does: If the classloader (e.g. the context classloader) is null, it uses the system classloader. The exception on null classloader was removed. The patch then also adds some null checks, so the "fallback" case is only used if both possible loaders are != null. If all class loaders are null, the system loader is used, which should never happen, as Lucene is not part of rt.jar. I think this is ready. Unfortunately we had some overlap, Christian
        Hide
        Uwe Schindler added a comment -

        Sorry, again a new patch.

        Now the case where the context class loader is null is handled correctly.

        Show
        Uwe Schindler added a comment - Sorry, again a new patch. Now the case where the context class loader is null is handled correctly.
        Hide
        Uwe Schindler added a comment -
        Show
        Uwe Schindler added a comment - Just for reference: see line 336+ of http://www.docjar.com/html/api/java/util/ServiceLoader.java.html
        Hide
        Christian Kohlschütter added a comment - - edited

        Overlap and coverage, Uwe
        Looks good to me!

        Nit: What you could do to be 100% safe that we're using the "correct" ClassLoader is to check for loader==null in SPIClassIterator and assign it to ClassLoader.getSystemClassLoader() in this case.

        Show
        Christian Kohlschütter added a comment - - edited Overlap and coverage, Uwe Looks good to me! Nit: What you could do to be 100% safe that we're using the "correct" ClassLoader is to check for loader==null in SPIClassIterator and assign it to ClassLoader.getSystemClassLoader() in this case.
        Hide
        Uwe Schindler added a comment -

        I also opened LUCENE-4823 to make the reloading (which is done on Solr startup to load codecs from plugin folders) more centralized. This is not really related but might move the isParentClassLoader helper method into the new base class for all SPILoaders (and hide it).

        Show
        Uwe Schindler added a comment - I also opened LUCENE-4823 to make the reloading (which is done on Solr startup to load codecs from plugin folders) more centralized. This is not really related but might move the isParentClassLoader helper method into the new base class for all SPILoaders (and hide it).
        Hide
        Uwe Schindler added a comment -

        Nit: What you could do to be 100% safe that we're using the "correct" ClassLoader is to check for loader==null in SPIClassIterator and assign it to ClassLoader.getSystemClassLoader() in this case.

        I want to keep as close to Java's original. This is not a problem at all: Class.forName(name, ..., NULL) loads automatically using the bootstrap / system loader.

        Show
        Uwe Schindler added a comment - Nit: What you could do to be 100% safe that we're using the "correct" ClassLoader is to check for loader==null in SPIClassIterator and assign it to ClassLoader.getSystemClassLoader() in this case. I want to keep as close to Java's original. This is not a problem at all: Class.forName(name, ..., NULL) loads automatically using the bootstrap / system loader.
        Hide
        Uwe Schindler added a comment -

        Hi Christian,
        after some investigation I found out why the java.util.ServiceLoader simply uses "null" but still always uses the system classloader: The reason is simple: java.util.ServiceLoader is itsself loaded by the system classloader (as its in rt.jar), so getCallerClassLoader() inside Class.forName() returns the system classloader. So there is no need to have a special case in java.

        The attached patch uses the system classloader if SPIClassIterator gets a null one, so its 100% consistent.

        But the change is not really needed, as Lucene's classloader is always != null, because its no system class so we will never get into this detail. I just changed it to be consistent.

        Please note: getSystemClassLoader may return null - I am not sure if this ever happens, the java code talks about compilation of rt.jar and such funny things, so i think the null checks in the Class implementation is more for bootstrapping the JVM, user code will never see a null system classloader.

        In either case, there is no problem with the current code if it returns null: its never called (haha) and it uses Class.forName, which supports null as classloader.

        Show
        Uwe Schindler added a comment - Hi Christian, after some investigation I found out why the java.util.ServiceLoader simply uses "null" but still always uses the system classloader: The reason is simple: java.util.ServiceLoader is itsself loaded by the system classloader (as its in rt.jar), so getCallerClassLoader() inside Class.forName() returns the system classloader. So there is no need to have a special case in java. The attached patch uses the system classloader if SPIClassIterator gets a null one, so its 100% consistent. But the change is not really needed, as Lucene's classloader is always != null, because its no system class so we will never get into this detail. I just changed it to be consistent. Please note: getSystemClassLoader may return null - I am not sure if this ever happens, the java code talks about compilation of rt.jar and such funny things, so i think the null checks in the Class implementation is more for bootstrapping the JVM, user code will never see a null system classloader. In either case, there is no problem with the current code if it returns null: its never called (haha) and it uses Class.forName, which supports null as classloader.
        Hide
        Christian Kohlschütter added a comment -

        Hi Uwe,

        Thanks for investigating this!

        Lucene is repeatedly hitting Java's inner and outer boundaries, also in this case.
        I agree that we're safe here. And if not, there's this issue's comments section for posterity

        Show
        Christian Kohlschütter added a comment - Hi Uwe, Thanks for investigating this! Lucene is repeatedly hitting Java's inner and outer boundaries, also in this case. I agree that we're safe here. And if not, there's this issue's comments section for posterity
        Hide
        Uwe Schindler added a comment -

        Thanks Christian!

        Show
        Uwe Schindler added a comment - Thanks Christian!
        Hide
        Commit Tag Bot added a comment -

        [branch_4x commit] Uwe Schindler
        http://svn.apache.org/viewvc?view=revision&revision=1455863

        Merged revision(s) 1455860 from lucene/dev/trunk:
        LUCENE-4713: The SPI components used to load custom codecs or analysis components were fixed to also scan the Lucene ClassLoader in addition to the context ClassLoader, so Lucene is always able to find its own codecs. The special case of a null context ClassLoader is now also supported.

        Show
        Commit Tag Bot added a comment - [branch_4x commit] Uwe Schindler http://svn.apache.org/viewvc?view=revision&revision=1455863 Merged revision(s) 1455860 from lucene/dev/trunk: LUCENE-4713 : The SPI components used to load custom codecs or analysis components were fixed to also scan the Lucene ClassLoader in addition to the context ClassLoader, so Lucene is always able to find its own codecs. The special case of a null context ClassLoader is now also supported.
        Hide
        Commit Tag Bot added a comment -

        [trunk commit] Uwe Schindler
        http://svn.apache.org/viewvc?view=revision&revision=1455860

        LUCENE-4713: The SPI components used to load custom codecs or analysis components were fixed to also scan the Lucene ClassLoader in addition to the context ClassLoader, so Lucene is always able to find its own codecs. The special case of a null context ClassLoader is now also supported.

        Show
        Commit Tag Bot added a comment - [trunk commit] Uwe Schindler http://svn.apache.org/viewvc?view=revision&revision=1455860 LUCENE-4713 : The SPI components used to load custom codecs or analysis components were fixed to also scan the Lucene ClassLoader in addition to the context ClassLoader, so Lucene is always able to find its own codecs. The special case of a null context ClassLoader is now also supported.
        Hide
        Uwe Schindler added a comment -

        As Lucene 4.2.1 is coming, I backported to 4.2 branch in revision: 1457695

        Show
        Uwe Schindler added a comment - As Lucene 4.2.1 is coming, I backported to 4.2 branch in revision: 1457695
        Hide
        Commit Tag Bot added a comment -

        [branch_4x commit] Uwe Schindler
        http://svn.apache.org/viewvc?view=revision&revision=1457699

        Merged revision(s) 1457697 from lucene/dev/trunk:
        LUCENE-4713: Move changes entry

        Show
        Commit Tag Bot added a comment - [branch_4x commit] Uwe Schindler http://svn.apache.org/viewvc?view=revision&revision=1457699 Merged revision(s) 1457697 from lucene/dev/trunk: LUCENE-4713 : Move changes entry
        Hide
        Commit Tag Bot added a comment -

        [trunk commit] Uwe Schindler
        http://svn.apache.org/viewvc?view=revision&revision=1457697

        LUCENE-4713: Move changes entry

        Show
        Commit Tag Bot added a comment - [trunk commit] Uwe Schindler http://svn.apache.org/viewvc?view=revision&revision=1457697 LUCENE-4713 : Move changes entry
        Hide
        Uwe Schindler added a comment -

        Closed after release.

        Show
        Uwe Schindler added a comment - Closed after release.

          People

          • Assignee:
            Uwe Schindler
            Reporter:
            Christian Kohlschütter
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development