Bug 968049 - org.jboss.resteasy.logging.Logger uses TCCL to load logging implementation
org.jboss.resteasy.logging.Logger uses TCCL to load logging implementation
Status: CLOSED CURRENTRELEASE
Product: JBoss Enterprise Application Platform 6
Classification: JBoss
Component: RESTEasy (Show other bugs)
6.2.0
Unspecified Unspecified
unspecified Severity unspecified
: ER10
: EAP 6.3.0
Assigned To: Weinan Li
Katerina Novotna
:
Depends On:
Blocks: 972941
  Show dependency treegraph
 
Reported: 2013-05-28 17:20 EDT by Kyle Lape
Modified: 2014-10-25 08:36 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-08-06 10:38:59 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)


External Trackers
Tracker ID Priority Status Summary Last Updated
JBoss Issue Tracker RESTEASY-743 Major Closed Thread.currentThread().getContextClassLoader() in Logger is problematic in OSGi environment 2016-03-10 05:56 EST

  None (edit)
Description Kyle Lape 2013-05-28 17:20:14 EDT
Description of problem:
Resteasy should use its own classloader to load classes.

Example scenario where this is a problem:
Login module depends on resteasy.  When login module is invoked, the TCCL is set to the deployment's CL, which does not depend on resteasy.  This causes a CNFE in org.jboss.resteasy.logging.Logger.setLoggerType()
Comment 1 Pavel Janousek 2013-06-12 04:48:04 EDT
Can be a such change of behavior acceptable for CP (cumulative patch) release?
Comment 2 dereed 2013-06-12 05:07:17 EDT
There should not be any backwards compatibility issues.

The class being loaded is in the Resteasy module.
Before the fix, it was loaded indirectly from the Resteasy module, as a dependency of the TCCL.
With the fix, it is loaded directly from the Resteasy module.

There are only 2 use cases that will change behavior:
1.  TCCL does not have a dependency on the Resteasy module.
  This is the use case this BZ is intended to correct.
2.  TCCL contains a different version of Resteasy.
  The previous behavior was wrong, and likely didn't work at all.
Comment 3 Pavel Janousek 2013-06-12 05:14:31 EDT
Thank you Dennis for explanation. I agree with you and I'm going to set QA ACK+.

Is there a reproducer of CNFE or more detailed info how to reproduce the issue? It might be the quickest way how to verify the fix without too much additional coding on our side. Thank you.
Comment 4 Weinan Li 2013-06-25 10:59:40 EDT
Reported to upstream: https://issues.jboss.org/browse/RESTEASY-901

It's targeted at 2.3.7.Final
Comment 6 Weinan Li 2013-08-25 23:39:10 EDT
Fixed in RESTEasy 2.3.7.Final
Comment 7 Petr Sakař 2013-10-18 07:41:38 EDT
class org.jboss.resteasy.logging.Logger from resteasy-jaxrs-2.3.7.Final-redhat-2 is still using TCCL in its static initialization block, which leads to login module failure in case when TCCL is not set (is null)

   private static boolean classInClasspath(String className)
   {
      try
      {
         return Thread.currentThread().getContextClassLoader().loadClass(className) != null;
      }
      catch (ClassNotFoundException e)
      {
         return false;
      }

   }

   static
   {
      LoggerType type = LoggerType.JUL;

      if (classInClasspath("org.apache.log4j.Logger"))
      {
         type = LoggerType.LOG4J;
      }
      else if (classInClasspath("org.slf4j.Logger"))
      {
         type = LoggerType.SLF4J;
      }

      setLoggerType(type);
   }
Comment 9 Weinan Li 2014-07-07 06:09:45 EDT
Fixed in 2.3.8:

   static
   {
      LoggerType type = LoggerType.JUL;

      if (classInClasspath("org.apache.log4j.Logger"))
      {
         type = LoggerType.LOG4J;
      }
      else if (classInClasspath("org.slf4j.Logger"))
      {
         type = LoggerType.SLF4J;
      }

      setLoggerType(type);
   }


...

   public static void setLoggerType(LoggerType loggerType)
   {
      try
      {
         Class loggerClass = null;
         if (loggerType == LoggerType.JUL)
         {
            loggerClass = Logger.class.getClassLoader().loadClass("org.jboss.resteasy.logging.impl.JULLogger"); <--- defining classloader used
         }
         else if (loggerType == LoggerType.LOG4J)
         {
            loggerClass = Logger.class.getClassLoader().loadClass("org.jboss.resteasy.logging.impl.Log4jLogger"); <--- defining classloader used
         }
         else if (loggerType == LoggerType.SLF4J)
         {
            loggerClass = Logger.class.getClassLoader().loadClass("org.jboss.resteasy.logging.impl.Slf4jLogger"); <--- defining classloader used
         }
         if (loggerClass == null)
            throw new RuntimeException("Could not match up an implementation for LoggerType: " + loggerType);
         loggerConstructor = loggerClass.getDeclaredConstructor(String.class);
      }
      catch (ClassNotFoundException e)
      {
         throw new RuntimeException(e);
      }
      catch (NoSuchMethodException e)
      {
         throw new RuntimeException(e);
      }

   }

btw, I saw this is also fixed in 2.3.7.1. Because the fix is in 'setLoggerType()' method, not the 'classInClasspath()' method. The classInClasspath shouldn't be changed as trying to find the correct logger framework is expected.
Comment 10 Katerina Novotna 2014-07-10 05:41:40 EDT
Hi Weinan,

I think Petr meant something else in his comment 7. In the fix we use Logger classloader instead of TCCL classloader: 

Thread.currentThread().getContextClassLoader() -> Logger.class.getClassLoader()

Which is ok. The problem is in classInClasspath() method we still use TCCL to check whether specified class is in the class path. Shouldn't we use again Logger classloader?
Comment 11 Weinan Li 2014-07-10 05:45:20 EDT
(In reply to Katerina Novotna from comment #10)
> Hi Weinan,
> 
> I think Petr meant something else in his comment 7. In the fix we use Logger
> classloader instead of TCCL classloader: 
> 
> Thread.currentThread().getContextClassLoader() ->
> Logger.class.getClassLoader()
> 
> Which is ok. The problem is in classInClasspath() method we still use TCCL
> to check whether specified class is in the class path. Shouldn't we use
> again Logger classloader?

As I understand we use TCCL to find the proper Logger in WildFly/EAP environment:

...
classInClasspath("org.apache.log4j.Logger") <-- TCCL used to find logger framework in env
...

And then we use class defining class loader to load RESTEasy log module:

loggerClass = Logger.class.getClassLoader().loadClass("org.jboss.resteasy.logging.impl.Slf4jLogger"); <--- defining classloader used to load relative resteasy logger.

Ron, could you please help to verify this?
Comment 12 Katerina Novotna 2014-07-11 09:11:53 EDT
Verification postponed to ER10, because we are waiting for information.
Comment 13 Katerina Novotna 2014-07-17 10:25:30 EDT
I thought about it second time and I see it like this:

If Thread.currentThread().getContextClassLoader() in classInClasspath method returns null then calling loadClass() on null would cause NullPointerException, therefore using Logger.class.getClassLoader().loadClass() will better way to handle this.

For example:

ClassLoader cl = Thread.currentThread().getContextClassLoader();
if (cl == null) cl = Logger.class.getClassLoader(); // fallback
Class clazz = cl.loadClass(name);
Comment 14 dereed 2014-07-17 19:26:53 EDT
The issue in #7 is not related to this BZ (loading the RestEasy classes from RestEasy's classloader instead of the TCCL).
That part is currently working as designed (loading the logger impl from the TCCL)

If you believe #13 is an issue, that should be a separate BZ.
Comment 15 Katerina Novotna 2014-07-18 05:44:32 EDT
Thank you Dennis for your comment.

I will then close this bz.
Comment 16 Katerina Novotna 2014-07-18 06:39:30 EDT
Verified in EAP 6.3.0.ER10.

Note You need to log in before you can comment on or make changes to this bug.