Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0.4
    • Fix Version/s: 1.1.0
    • Labels:
      None
    • Environment:

      Operating System: other
      Platform: Other

      Description

      I separated this request from 35774 as it seems to provide a viable and easy
      solution to a big number of ClassLoader-related problems of using JCL in shared
      environments. The solution relies on having an option (configured via system
      properties) to disable TCCL usage and only use LogFactory's ClassLoader. When
      TCCL is disabled, logger implementation and caching should be performed in the
      JCL's ClassLoader only (cache will be reduced to a single record). This would
      remove any memory leak issues as well as other inconsitencies (see 35774 for
      some samples).

      There're two main scenarios how JCL is used in the shared environment: a)
      deployed in the shared class loader (shared for Tomcat and UCL for JBoss), b)
      deployed in the application's class loader in the child-first ClassLoader
      setup. In both cases this solution works well and disabled TCCL could even be a
      reasonable default (though it depends on other uses). In case of deployment
      within application's classloader (case [b]), two versions of JCL can coexist
      redirecting logs for the application's classes to application's LogFactory
      instance and shared classes to shared LogFactory.

        Activity

        Hide
        Simon Kitching added a comment -

        I have a proposal. I've implemented what's described below, and have unit tests
        mostly working for it so it's ready to go pretty soon if there are no objections.

        == part 1 ==

        When loading commons-logging.properties files, currently the first one found in
        the class is used.

        Instead, I propose that commons-logging.properties files can contain a
        "priority=n" . Files without an entry default to priority=0. The priority value
        is a floating-point number. When loading properties files, every file in the
        classpath is checked, and the one with the highest priority is used.

        This allows webapps to provide a config file, but for the container
        administrator to override these by placing a file of higher priority in the
        shared path. A webapp can then override that if it wants by defining an even
        higher priority.

        This isn't a major performance hit as it is done only once per distinct TCCL (ie
        per webapp in a container).

        Priority numbers aren't perfect as they do require some "coordination" between
        the different levels. However I don't think there's anything better in this case.

        == part 2 ==

        Allow commons-logging.properties files to contain a key
        use_tccl=false
        If this is present, then for the current TCCL all loading is done via the
        classloader that loaded the LogFactory class.

        == effect ==

        Together, these features allow the following:
        (a)
        A single webapp can disable tccl logging for itself by having a
        commons-logging.properties file with use_tccl=false.

        (b)
        A container administrator can disable TCCL-based logging for all webapps by
        simply placing a commons-logging.properties file in a shared classpath location
        with priority=10 and use_tccl=false. This will override any settings in any
        webapp. However a specific webapp can override this again with a higher-priority
        value for its config file.

        At a technical level, there will still be a LogFactoryImpl for each distinct
        TCCL, keyed by the TCCL. However a call to LogFactory.getLog made when the
        current tccl matches one associated with a "use_tccl=false" configuration will
        always return a concrete Log class that is loaded via the same classloader that
        loaded the LogFactory class.

        Opinions?

        Show
        Simon Kitching added a comment - I have a proposal. I've implemented what's described below, and have unit tests mostly working for it so it's ready to go pretty soon if there are no objections. == part 1 == When loading commons-logging.properties files, currently the first one found in the class is used. Instead, I propose that commons-logging.properties files can contain a "priority=n" . Files without an entry default to priority=0. The priority value is a floating-point number. When loading properties files, every file in the classpath is checked, and the one with the highest priority is used. This allows webapps to provide a config file, but for the container administrator to override these by placing a file of higher priority in the shared path. A webapp can then override that if it wants by defining an even higher priority. This isn't a major performance hit as it is done only once per distinct TCCL (ie per webapp in a container). Priority numbers aren't perfect as they do require some "coordination" between the different levels. However I don't think there's anything better in this case. == part 2 == Allow commons-logging.properties files to contain a key use_tccl=false If this is present, then for the current TCCL all loading is done via the classloader that loaded the LogFactory class. == effect == Together, these features allow the following: (a) A single webapp can disable tccl logging for itself by having a commons-logging.properties file with use_tccl=false. (b) A container administrator can disable TCCL-based logging for all webapps by simply placing a commons-logging.properties file in a shared classpath location with priority=10 and use_tccl=false. This will override any settings in any webapp. However a specific webapp can override this again with a higher-priority value for its config file. At a technical level, there will still be a LogFactoryImpl for each distinct TCCL, keyed by the TCCL. However a call to LogFactory.getLog made when the current tccl matches one associated with a "use_tccl=false" configuration will always return a concrete Log class that is loaded via the same classloader that loaded the LogFactory class. Opinions?
        Hide
        Simon Kitching added a comment -

        Ok, as there were no objections I have committed this new feature.

        As of r370031, dropping a file named commons-logging.properties into the shared
        classpath containing the following entries will disable TCCL loading completely:

        priority=10
        use_tccl=false

        I would appreciate it if people could test this in live systems that have
        previously experienced problems.

        Show
        Simon Kitching added a comment - Ok, as there were no objections I have committed this new feature. As of r370031, dropping a file named commons-logging.properties into the shared classpath containing the following entries will disable TCCL loading completely: priority=10 use_tccl=false I would appreciate it if people could test this in live systems that have previously experienced problems.
        Hide
        Simon Kitching added a comment -

        Hi Dimitry,

        Release candidate 1 of commons-logging 1.1 has now been created:
        http://people.apache.org/~rdonkin/commons-logging/

        Could you please check whether this meets your requirements?

        Show
        Simon Kitching added a comment - Hi Dimitry, Release candidate 1 of commons-logging 1.1 has now been created: http://people.apache.org/~rdonkin/commons-logging/ Could you please check whether this meets your requirements?
        Hide
        rdonkin@apache.org added a comment -

        Please test with latest release candidate

        Show
        rdonkin@apache.org added a comment - Please test with latest release candidate
        Hide
        Simon Kitching added a comment -

        As there's been no response I'll presume the implemented fix is satisfactory.

        Show
        Simon Kitching added a comment - As there's been no response I'll presume the implemented fix is satisfactory.

          People

          • Assignee:
            Simon Kitching
            Reporter:
            Dimitry E Voytenko
          • Votes:
            1 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development