XalanJ2
  1. XalanJ2
  2. XALANJ-1402

Incorrect usage of context ClassLoader for static field initialization

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Critical Critical
    • Resolution: Unresolved
    • Affects Version/s: 2.5
    • Fix Version/s: None
    • Component/s: Xalan
    • Security Level: No security risk; visible to anyone (Ordinary problems in Xalan projects. Anybody can view the issue.)
    • Labels:
      None
    • Environment:
      Operating System: All
      Platform: All

      Description

      Several classes read resources through thread context ClassLoader. When it is
      right thing for factories, it it is definitely not right for static field
      initialization. Due to this it is problematic for me to use Xalan as Eclipse
      plugin. This might be issue in other environments with many ClassLoaders too.

      These two classes should not use context Classloader:
      org.apache.xalan.serialize.Encodings
      org.apache.xalan.templates.OutputProperties

      Instead they should simply read properties from their OWN Classloader as
      org.apache.xalan.serialize.CharInfo does:
      ClassLoader cl = CharInfo.class.getClassLoader();

      There are one more class:
      org.apache.xalan.processor.TransformerFactoryImpl
      It does "System.setProperties(systemProps)" in it's static initialization!
      Actually it does'nt set anything what could be needed except sax-driver (which
      should be defined in META-INF/services by default!), and I think it is something
      from legacy code and must be removed now (to remove possible side effects)...

        Activity

        Hide
        Gary L Peskin added a comment -

        Can you please explain why Encodings and OutputProperties should not use the
        context ClassLoader?

        Show
        Gary L Peskin added a comment - Can you please explain why Encodings and OutputProperties should not use the context ClassLoader?
        Hide
        Ilene Seelemann added a comment -

        Also, can you please explain why it's wrong to set system properties in a
        static initializer. Thanks.

        Show
        Ilene Seelemann added a comment - Also, can you please explain why it's wrong to set system properties in a static initializer. Thanks.
        Hide
        Igor Malinin added a comment -

        Encodings and OutputProperties:
        -------------------------------

        I could ask opposite question - why they should?
        These classes and their properties are integral part. They all are always in the
        same classloader! Java doesn't look for referenced classes in context
        classloader, then why xalan should look for properties in some other
        classloader. When it is used together with static fields it becomes madness.
        Context ClassLoader is PER-THREAD (!!!), static fields belong to all threads
        (!!!) and initialized only onece (from random CL!!!). In environment with
        several classloaders (Eclipse platform, application-servers, etc.) if someone
        does not properly initialized classloader on first call, then others will not be
        able to use Xalan, even if you only use JAXP APIs. It is a violation of JAXP
        specification as I understand it! In such multi-classloader environments Xalan
        usualy belongs to some custom classloader, but context ClassLoader is system
        ClassLoader. Then Xalan doesn't find resources, or finds incorrect ones (from
        older version of Xalan in my case).

        System Properties:
        ------------------

        Why Xalan should replace SAX driver property to
        org.xml.sax.driver=org.apache.xerces.parsers.SAXParser? What if I would like
        other SAX Parser and have set it through "java -Dorg.xml.sax.driver=..."? I
        wouldn't get it ant it is not what programmers expect. And why it does look a
        properties file through thread context ClassLoader to initialize static data
        (same issue as with Encodings and OutputProperties)?

        Show
        Igor Malinin added a comment - Encodings and OutputProperties: ------------------------------- I could ask opposite question - why they should? These classes and their properties are integral part. They all are always in the same classloader! Java doesn't look for referenced classes in context classloader, then why xalan should look for properties in some other classloader. When it is used together with static fields it becomes madness. Context ClassLoader is PER-THREAD (!!!), static fields belong to all threads (!!!) and initialized only onece (from random CL!!!). In environment with several classloaders (Eclipse platform, application-servers, etc.) if someone does not properly initialized classloader on first call, then others will not be able to use Xalan, even if you only use JAXP APIs. It is a violation of JAXP specification as I understand it! In such multi-classloader environments Xalan usualy belongs to some custom classloader, but context ClassLoader is system ClassLoader. Then Xalan doesn't find resources, or finds incorrect ones (from older version of Xalan in my case). System Properties: ------------------ Why Xalan should replace SAX driver property to org.xml.sax.driver=org.apache.xerces.parsers.SAXParser? What if I would like other SAX Parser and have set it through "java -Dorg.xml.sax.driver=..."? I wouldn't get it ant it is not what programmers expect. And why it does look a properties file through thread context ClassLoader to initialize static data (same issue as with Encodings and OutputProperties)?
        Hide
        Ilene Seelemann added a comment -

        System Properties
        -----------------

        Taking the SAX Driver property from XSLTInfo.properties should be faster than
        using the Jar Service Provider Mechanism to find the class. And, if the system
        property is already set, we do not replace it with the value in
        XSLTInfo.properties:

        while (propEnum.hasMoreElements())

        { String prop = (String) propEnum.nextElement(); if (!systemProps.containsKey(prop)) systemProps.put(prop, props.getProperty(prop)); }
        Show
        Ilene Seelemann added a comment - System Properties ----------------- Taking the SAX Driver property from XSLTInfo.properties should be faster than using the Jar Service Provider Mechanism to find the class. And, if the system property is already set, we do not replace it with the value in XSLTInfo.properties: while (propEnum.hasMoreElements()) { String prop = (String) propEnum.nextElement(); if (!systemProps.containsKey(prop)) systemProps.put(prop, props.getProperty(prop)); }
        Hide
        Gary L Peskin added a comment -

        Encodings and OutputProperties
        ------------------------------

        Static values are maintained for each classloader. Can you please detail the
        use case that you encountered on the Eclipse platform. Where are your
        xalan.jar files? How many are there? How are you invoking XalanJ? I would
        very much like to study your problem in detail. So, can you please provide
        enough information and details so that I can reproduce it.

        Thanks,
        Gary

        Show
        Gary L Peskin added a comment - Encodings and OutputProperties ------------------------------ Static values are maintained for each classloader. Can you please detail the use case that you encountered on the Eclipse platform. Where are your xalan.jar files? How many are there? How are you invoking XalanJ? I would very much like to study your problem in detail. So, can you please provide enough information and details so that I can reproduce it. Thanks, Gary
        Hide
        Igor Malinin added a comment -

        Encodings and OutputProperties
        ------------------------------

        > Static values are maintained for each classloader.

        For each classloader, but not for each thread!!!

        I'm trying to setup following plugin structure (every plugin in own classloader):

        • solareclipse.ext.jaxp:
          JAXP API only. Implementation based on xml-commons and factories have additional
          changes to be able to find implementation through Eclipse extension-points.
        • solareclipse.ext.xerces (depends on JAXP):
          Xerces implementation of JAXP parser.
        • solareclipse.ext.xalan (depends on JAXP and Xerces):
          Xalan implementation of JAXP transformer.

        All works well if SUN JVM 1.3 is used.

        SUN JVM 1.4 (And IBM 1.3 as I remember) contains Xalan included into
        distribution. To be able to use non-system version of Xalan I am implementing
        classloading extension for 'endorsed' libraries which hides system supplied
        xalan and replaces it with own version (see
        http://bugs.eclipse.org/bugs/show_bug.cgi?id=29007 and core-dev mailing list).
        Note that Xalan could be replaced through JVM endorsed mechanism, but only once

        • you cannot use versioning and always locked to single version of Xalan in one
          JVM - i.e. if you have two plugins based on different versions of Xalan then you
          wouldn't be able to use them together if JVM has included Xalan! Same should
          apply to some possible application-server implementations...

        I'm quite succesfull in doing this extension, but Xalan refuses to work. Since
        context ClassLoader is system classloader by default, Encodings and
        OutputProperties are initialized from wrong resources. There is workaround -
        always set context ClassLoader before any use of Xalan and/or JAXP. But it is
        not convenient especially in case when you use exclusively JAXP API. And more,
        if you did not set it on first call, others will newer work even if you set
        appropriate context ClassLoader!

        Using of context ClassLoader in static initialization is bad practice in any
        case, because of dynamic nature of context ClassLoader; example: you cannot call
        non-static class methods from static methods! Initialization are done here in
        unpredictable way!!!

        System Properties
        -----------------

        > Taking the SAX Driver property from XSLTInfo.properties should be faster
        > than using the Jar Service Provider Mechanism to find the class.

        This doesn't affect me. I can live with it, but...
        I think that it is responsibility of application initialization to set System
        properties, not a library responsibility! User of the library expects
        conformance in first place, i.e. lookup through services! If someone needs
        performance, he is able to set this property themselvs!

        Main problem I see here is usage of context ClassLoader here for STATIC
        initialization! These properties is bound to Xalan distribution, it is not an
        extension, and should be looked up through own Claslloader, but not some random
        thread context!

        And second (not about sax.driver) - there is only one instance of System
        properties, not per-classloader. If there is several classloaders with different
        versions of Xalan, properties that are set doesn't reflect reality... Then why
        to set them at all? Xalan itself doesn't use them!

        Show
        Igor Malinin added a comment - Encodings and OutputProperties ------------------------------ > Static values are maintained for each classloader. For each classloader, but not for each thread!!! I'm trying to setup following plugin structure (every plugin in own classloader): solareclipse.ext.jaxp: JAXP API only. Implementation based on xml-commons and factories have additional changes to be able to find implementation through Eclipse extension-points. solareclipse.ext.xerces (depends on JAXP): Xerces implementation of JAXP parser. solareclipse.ext.xalan (depends on JAXP and Xerces): Xalan implementation of JAXP transformer. All works well if SUN JVM 1.3 is used. SUN JVM 1.4 (And IBM 1.3 as I remember) contains Xalan included into distribution. To be able to use non-system version of Xalan I am implementing classloading extension for 'endorsed' libraries which hides system supplied xalan and replaces it with own version (see http://bugs.eclipse.org/bugs/show_bug.cgi?id=29007 and core-dev mailing list). Note that Xalan could be replaced through JVM endorsed mechanism, but only once you cannot use versioning and always locked to single version of Xalan in one JVM - i.e. if you have two plugins based on different versions of Xalan then you wouldn't be able to use them together if JVM has included Xalan! Same should apply to some possible application-server implementations... I'm quite succesfull in doing this extension, but Xalan refuses to work. Since context ClassLoader is system classloader by default, Encodings and OutputProperties are initialized from wrong resources. There is workaround - always set context ClassLoader before any use of Xalan and/or JAXP. But it is not convenient especially in case when you use exclusively JAXP API. And more, if you did not set it on first call, others will newer work even if you set appropriate context ClassLoader! Using of context ClassLoader in static initialization is bad practice in any case, because of dynamic nature of context ClassLoader; example: you cannot call non-static class methods from static methods! Initialization are done here in unpredictable way!!! System Properties ----------------- > Taking the SAX Driver property from XSLTInfo.properties should be faster > than using the Jar Service Provider Mechanism to find the class. This doesn't affect me. I can live with it, but... I think that it is responsibility of application initialization to set System properties, not a library responsibility! User of the library expects conformance in first place, i.e. lookup through services! If someone needs performance, he is able to set this property themselvs! Main problem I see here is usage of context ClassLoader here for STATIC initialization! These properties is bound to Xalan distribution, it is not an extension, and should be looked up through own Claslloader, but not some random thread context! And second (not about sax.driver) - there is only one instance of System properties, not per-classloader. If there is several classloaders with different versions of Xalan, properties that are set doesn't reflect reality... Then why to set them at all? Xalan itself doesn't use them!
        Hide
        Ilene Seelemann added a comment -

        Hmmm...I'm just wondering why we need a classloader at all, to load properties
        from a file. Could we do something like what is in the
        SecuritySupport12.getFileInputStream() class in xml-commons:

        public FileInputStream getFileInputStream(final File file)
        throws FileNotFoundException
        {
        try {
        return (FileInputStream)
        AccessController.doPrivileged(new PrivilegedExceptionAction() {
        public Object run() throws FileNotFoundException

        { return new FileInputStream(file); }

        });
        } catch (PrivilegedActionException e)

        { throw (FileNotFoundException)e.getException(); }

        }

        and then do:

        FileInputStream fis = ss.getFileInputStream(file1);
        Properties properties;
        properties.load(fis);

        ???

        Of course, with similar code for the case that the JDK is not 1.2 or higher.

        Show
        Ilene Seelemann added a comment - Hmmm...I'm just wondering why we need a classloader at all, to load properties from a file. Could we do something like what is in the SecuritySupport12.getFileInputStream() class in xml-commons: public FileInputStream getFileInputStream(final File file) throws FileNotFoundException { try { return (FileInputStream) AccessController.doPrivileged(new PrivilegedExceptionAction() { public Object run() throws FileNotFoundException { return new FileInputStream(file); } }); } catch (PrivilegedActionException e) { throw (FileNotFoundException)e.getException(); } } and then do: FileInputStream fis = ss.getFileInputStream(file1); Properties properties; properties.load(fis); ??? Of course, with similar code for the case that the JDK is not 1.2 or higher.
        Hide
        Gary L Peskin added a comment -

        Ilene –

        These "files" are distributed in the .jar file and are found in the classpath.
        I don't believe your method would follow the same search rules.

        Gary

        Show
        Gary L Peskin added a comment - Ilene – These "files" are distributed in the .jar file and are found in the classpath. I don't believe your method would follow the same search rules. Gary
        Hide
        Ilene Seelemann added a comment -

        Oops...yeah, that makes sense.

        Show
        Ilene Seelemann added a comment - Oops...yeah, that makes sense.
        Hide
        Igor Malinin added a comment -

        Created an attachment (id=4748)
        diff for org.apache.xalan.serialize.Encodings, org.apache.xalan.templates.OutputProperties and org.apache.xalan.processor.TransformerFactoryImpl

        Show
        Igor Malinin added a comment - Created an attachment (id=4748) diff for org.apache.xalan.serialize.Encodings, org.apache.xalan.templates.OutputProperties and org.apache.xalan.processor.TransformerFactoryImpl
        Hide
        Gary L Peskin added a comment -

        I am very opposed to this proposed change as it is. It needs to be carefully
        evaluated in light of the class loading strategies used by various J2EE
        containers such as Tomcat (for servlets, anyway), WebSphere, and WebLogic. The
        proposed change prevents different web applications from having different
        copies of Encodings and OutputProperties and I think that might be desirable.

        I need to look at the classloading extension for Eclipse and understand how
        that works and why Eclipse can't set the thread context classloader, which is
        the way it should be handled, at my first impression.

        Show
        Gary L Peskin added a comment - I am very opposed to this proposed change as it is. It needs to be carefully evaluated in light of the class loading strategies used by various J2EE containers such as Tomcat (for servlets, anyway), WebSphere, and WebLogic. The proposed change prevents different web applications from having different copies of Encodings and OutputProperties and I think that might be desirable. I need to look at the classloading extension for Eclipse and understand how that works and why Eclipse can't set the thread context classloader, which is the way it should be handled, at my first impression.
        Hide
        Igor Malinin added a comment -

        The issue of this bug is not a change in ClassLoading strategy in general (a
        change in strategy is requested in bug #16675 with explanations why it doesn't
        change semantics of 'normal' classloaders). I try to summrize again the issue of
        this bug and why I'm absolutely shure it is incorrect. Read it carefully and
        you'll understand that I'm rigt:

        1. Any static field is one per ClassLoader. It initialized only once at first
        call. There might be any numbers of threads! Then which thread's context
        ClassLoader should be used for initialization?

        Answer could be only one - doesn't use context ClassLoader at all!!! The most
        natural way is to use current ClassLoader! And since resource is always bundled
        together with class (i.e. same ClassLoader) this way is absolutely right.

        2. It doesn't prevent different web applications from having different
        copies of Encodings and OutputProperties. If someone wants customized copies for
        Encodings and OutputProperties then:
        a) It is unlikely (since these properties standards-defined: W3C defined
        defaults which could be overriden in XSLT stylesheet).
        b) If it really required then this should be done some other 'STATIC' way (for
        example by placing appropriate .properties resources somewhere where classloader
        would look before xalan.jar, /WEB-INF/classes is consulted before any
        /WEB-INF/lib/*.jar in web containers so could be used for such owerride).
        c) Do not cache these classes and reload on every call, which is ineffective
        especially for [a].

        Draw attention that even in case with usage of context ClassLoader you MUST put
        overriden version somewhere where classloader would look before xalan.jar like
        described in [b]!!! I.e. your assumption that my change breaks this possibility
        is incorrect.

        In Eclipse you can't control Context ClassLoader in framework because there is
        no single source of control as in web-containers. Any plugin could do calls to
        any other... But angain it is a different issue - here we talk about static
        fields where any dependency on thead context is simply inappropriate!

        Show
        Igor Malinin added a comment - The issue of this bug is not a change in ClassLoading strategy in general (a change in strategy is requested in bug #16675 with explanations why it doesn't change semantics of 'normal' classloaders). I try to summrize again the issue of this bug and why I'm absolutely shure it is incorrect. Read it carefully and you'll understand that I'm rigt: 1. Any static field is one per ClassLoader. It initialized only once at first call. There might be any numbers of threads! Then which thread's context ClassLoader should be used for initialization? Answer could be only one - doesn't use context ClassLoader at all!!! The most natural way is to use current ClassLoader! And since resource is always bundled together with class (i.e. same ClassLoader) this way is absolutely right. 2. It doesn't prevent different web applications from having different copies of Encodings and OutputProperties. If someone wants customized copies for Encodings and OutputProperties then: a) It is unlikely (since these properties standards-defined: W3C defined defaults which could be overriden in XSLT stylesheet). b) If it really required then this should be done some other 'STATIC' way (for example by placing appropriate .properties resources somewhere where classloader would look before xalan.jar, /WEB-INF/classes is consulted before any /WEB-INF/lib/*.jar in web containers so could be used for such owerride). c) Do not cache these classes and reload on every call, which is ineffective especially for [a] . Draw attention that even in case with usage of context ClassLoader you MUST put overriden version somewhere where classloader would look before xalan.jar like described in [b] !!! I.e. your assumption that my change breaks this possibility is incorrect. In Eclipse you can't control Context ClassLoader in framework because there is no single source of control as in web-containers. Any plugin could do calls to any other... But angain it is a different issue - here we talk about static fields where any dependency on thead context is simply inappropriate!
        Hide
        Gary L Peskin added a comment -

        Your explanation of 2(b) is fine if there is a xalan.jar associated with each
        WebApp. I am referring to the case where there is one xalan.jar to be used by
        all webapps but different Encodings or OutputProperties.

        I'll take a closer look at the code regarding where the static field lies and
        repost here in a few days.

        Gary

        Show
        Gary L Peskin added a comment - Your explanation of 2(b) is fine if there is a xalan.jar associated with each WebApp. I am referring to the case where there is one xalan.jar to be used by all webapps but different Encodings or OutputProperties. I'll take a closer look at the code regarding where the static field lies and repost here in a few days. Gary
        Hide
        Igor Malinin added a comment -

        Just notified that my patches for the same issue but with Xerces were accepted
        and committed to CVS ( see bug #16509 ).

        Gary. The case you described is impossible in current code too (same one
        xalan.jar ClassLoader -> one static variable initialized frome one resource)...

        Show
        Igor Malinin added a comment - Just notified that my patches for the same issue but with Xerces were accepted and committed to CVS ( see bug #16509 ). Gary. The case you described is impossible in current code too (same one xalan.jar ClassLoader -> one static variable initialized frome one resource)...
        Hide
        Ilene Seelemann added a comment -

        Gary: Do you still have concerns about this patch? Igor has included these
        code changes with his patch for bug #16675. I'd like to get the generalized
        classloading mechanism (of 16675) integrated and wanted to find out your
        current thinking on this bug report. Thanks, Ilene.

        Show
        Ilene Seelemann added a comment - Gary: Do you still have concerns about this patch? Igor has included these code changes with his patch for bug #16675. I'd like to get the generalized classloading mechanism (of 16675) integrated and wanted to find out your current thinking on this bug report. Thanks, Ilene.
        Hide
        Brian Minchau added a comment -

        Igor,
        please verify that this issue is fixed in 2.7.0.

        Ilene put a comment back in Feb 2003 that some fixes about the patch refered to went into bugzilla #16675, which became XALANJ-411 in the transfer to JIRA. XALANJ-411 is resolved.

        I think this one is an issue anymore either. Please confirm.
        Thanks,
        Brian Minchau

        Show
        Brian Minchau added a comment - Igor, please verify that this issue is fixed in 2.7.0. Ilene put a comment back in Feb 2003 that some fixes about the patch refered to went into bugzilla #16675, which became XALANJ-411 in the transfer to JIRA. XALANJ-411 is resolved. I think this one is an issue anymore either. Please confirm. Thanks, Brian Minchau
        Hide
        Igor Malinin added a comment -

        I can confirm that it is not fixed and still an issue:

        Static Encoding._encodings is still initialized based on context classloader;
        Contents of static CharInfo.m_getCharInfoCache could be initialized based on context classloader too.

        Other resource loading seems to be fixed correctly:

        The code from OutputProperties has been moved to OutputPropertiesFactory where own classloader used;
        The code from TransformerFactoryImpl has been moved to FuncSystemProperty where it is not in static context anymore.

        PS. It is not related to XALANJ-411 in any way, probably wrong reference.

        Show
        Igor Malinin added a comment - I can confirm that it is not fixed and still an issue: Static Encoding._encodings is still initialized based on context classloader; Contents of static CharInfo.m_getCharInfoCache could be initialized based on context classloader too. Other resource loading seems to be fixed correctly: The code from OutputProperties has been moved to OutputPropertiesFactory where own classloader used; The code from TransformerFactoryImpl has been moved to FuncSystemProperty where it is not in static context anymore. PS. It is not related to XALANJ-411 in any way, probably wrong reference.
        Hide
        Henry Zongaro added a comment -

        Brian meant to write XALANJ-1411.

        Show
        Henry Zongaro added a comment - Brian meant to write XALANJ-1411 .
        Hide
        Alfredo Scotto added a comment -

        Right, probably there are more places you listed. I just opened a bug because when after I deployed my application, with xalan/xerces 2.7.1, on Websphere 6.0 its deployment manager will not work anymore and there are no way to deploy new applications. It's look like it starts to use the classes from my application and I know (because I tried to put those jars in endorsed) they are not good for WAS6.0.
        I also experienced strange problems with Eclipse with plugins using different version of xalan/xerces. I pointed to buggy plugin but now I know the problem is from our two X-guys.
        From what I saw there is same kind of "class finder" like the one in common-logging. It probably looks to much and without boundaries, just to create problems.

        Show
        Alfredo Scotto added a comment - Right, probably there are more places you listed. I just opened a bug because when after I deployed my application, with xalan/xerces 2.7.1, on Websphere 6.0 its deployment manager will not work anymore and there are no way to deploy new applications. It's look like it starts to use the classes from my application and I know (because I tried to put those jars in endorsed) they are not good for WAS6.0. I also experienced strange problems with Eclipse with plugins using different version of xalan/xerces. I pointed to buggy plugin but now I know the problem is from our two X-guys. From what I saw there is same kind of "class finder" like the one in common-logging. It probably looks to much and without boundaries, just to create problems.

          People

          • Assignee:
            Unassigned
            Reporter:
            Igor Malinin
          • Votes:
            1 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development