Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: Nightly Builds
    • Fix Version/s: 1.0.3
    • Labels:
      None
    • Environment:

      Operating System: All
      Platform: PC

      Description

      Trying to run this software on an EAServer, a NullPointerException is thrown in
      two different places within LogFactory.java. I was able to trace the problem
      in the source:
      1) private static LogFactory getCachedFactory(ClassLoader contextClassLoader)

      { LogFactory factory = null; if (contextClassLoader != null) factory = (LogFactory) factories.get(contextClassLoader); if (factory==null) factory = (LogFactory) factories.get(LogFactory.class.getClassLoader ()); return factory; }

      the factories.get(LogFactory.class.getClassLoader()) will cause a problem
      because LogFactory.class.GetClassLoader() is null (using jre 1.4) changing
      that part to a call to getContextClassLoader() would resolve this bug.

      2) in getFactory():

      // Fourth, try the fallback implementation class
      if (factory == null)

      { factory = newFactory(FACTORY_DEFAULT, LogFactory.class.getClassLoader()); }

      same deal here, replacing LogFactory.class.getClassLoader() with contextClass
      will fix.

      I don't know if there was a reason these were left as
      LogFactory.class.getClassLoader(), but getContextClassLoader(), will determine
      if this is the correct call to make (and it states that this is only
      appropriate in pre-jdk1.1), instead of just assuming it is the right call,
      causing NullPointerExceptions.

      Perhaps this wasn't noticed since it usually doesn't get to this fallback
      position, but on the application server, it does every time.

        Activity

        Hide
        Craig McClanahan added a comment -

        Given no further discussion on this since January (and no changes to the
        LogFactory code in question), I'm going to assume that it's been addressed.
        Please reopen the bug report if you find out something new.

        In addition, if you take the (very helpful, thanks!) step of proposing a patch
        to fix something, the most useful format is a "cvs diff -u". This gives the
        committer applying the patch unambiguous documentation of which CVS revision the
        patch is against, and the "-u" format provides enough context to evaluate the
        impact of the patch on a code review.

        Show
        Craig McClanahan added a comment - Given no further discussion on this since January (and no changes to the LogFactory code in question), I'm going to assume that it's been addressed. Please reopen the bug report if you find out something new. In addition, if you take the (very helpful, thanks!) step of proposing a patch to fix something, the most useful format is a "cvs diff -u". This gives the committer applying the patch unambiguous documentation of which CVS revision the patch is against, and the "-u" format provides enough context to evaluate the impact of the patch on a code review.
        Hide
        Philip Mitchell added a comment -

        Yes, I know...and it doesn't for some reason (now I'm as puzzled as you
        are...perhaps more). But thanks for the FYI, I now know just a little bit more
        than I did 5 minutes ago.

        Show
        Philip Mitchell added a comment - Yes, I know...and it doesn't for some reason (now I'm as puzzled as you are...perhaps more). But thanks for the FYI, I now know just a little bit more than I did 5 minutes ago.
        Hide
        Richard A. Sitze added a comment -

        Sorry if this is obvious, but I'm going to ask anyway: Were you expecting 'step
        four' to find YOUR factory? If so, that would explain the confusion. 'step
        four' finds the DEFAULT.

        FYI,

        In a J2SE environment where you are using CLASSPATH to point to commons-
        logging.jar and yourOwn.jar, then you are correct - using context class loader
        is OK. In a J2EE environment with commons-logging installed at a system level,
        and your code as a web application, then the class loader hierarchy looks
        (loosely) like:

        webAppClassLoader -> EARFileClassLoader -> SystemClassLoader -> ..

        So, if you use the context class loader, then you are unnecessarily walking up
        the tree to find something in SystemClassLoader. In this example,
        LogFactory.class.getClassLoader() == SystemClassLoader(), which is no more and
        no less than what is required to find the companion class in the same jar file
        with LogFactory. The 'extra logic' that makes all that happen is to avoid
        walking the class loader chain - maybe a small nit, but it's something.

        BTW, I incorrectly stated previously that you could overload the
        FACTORY_DEFAULT by placing a class in the lower [i.e webApp] classloader...
        default/standard behavior is to defer up the hierarchy for class resolution
        before looking at local class loader). As you pointed out, in your hierarchy
        it appears that it's found by the bootstrap class loader, which can be returned
        as null.. that's odd to my simple mind, but that's the documented (and
        functional) behavior.

        Show
        Richard A. Sitze added a comment - Sorry if this is obvious, but I'm going to ask anyway: Were you expecting 'step four' to find YOUR factory? If so, that would explain the confusion. 'step four' finds the DEFAULT. FYI, In a J2SE environment where you are using CLASSPATH to point to commons- logging.jar and yourOwn.jar, then you are correct - using context class loader is OK. In a J2EE environment with commons-logging installed at a system level, and your code as a web application, then the class loader hierarchy looks (loosely) like: webAppClassLoader -> EARFileClassLoader -> SystemClassLoader -> .. So, if you use the context class loader, then you are unnecessarily walking up the tree to find something in SystemClassLoader. In this example, LogFactory.class.getClassLoader() == SystemClassLoader(), which is no more and no less than what is required to find the companion class in the same jar file with LogFactory. The 'extra logic' that makes all that happen is to avoid walking the class loader chain - maybe a small nit, but it's something. BTW, I incorrectly stated previously that you could overload the FACTORY_DEFAULT by placing a class in the lower [i.e webApp] classloader... default/standard behavior is to defer up the hierarchy for class resolution before looking at local class loader). As you pointed out, in your hierarchy it appears that it's found by the bootstrap class loader, which can be returned as null.. that's odd to my simple mind, but that's the documented (and functional) behavior.
        Hide
        Philip Mitchell added a comment -

        Now that you point that out, I, too, am puzzled...I was able to make the code
        work by setting the system property to get it to point to the appropriate class
        file (and avoiding that whole fourth alternative)

        There may be something specific to my environment that is causing these
        troubles, but as I look at the code now, it SHOULD work, as you said. I still
        don't understand why not use contextClassLoader instead of the other, since
        getContextClassLoader() will have returned the appropriate call, saving
        newFactory() a lot of work, and saving unnecessary method calls and being
        consistent with the rest of the method, but there may be something beyond the
        scope of my problems that made you go with that approach, I do not know.

        Thank you for looking at it anyways, I will try to see if there's something
        else at my end.

        Show
        Philip Mitchell added a comment - Now that you point that out, I, too, am puzzled...I was able to make the code work by setting the system property to get it to point to the appropriate class file (and avoiding that whole fourth alternative) There may be something specific to my environment that is causing these troubles, but as I look at the code now, it SHOULD work, as you said. I still don't understand why not use contextClassLoader instead of the other, since getContextClassLoader() will have returned the appropriate call, saving newFactory() a lot of work, and saving unnecessary method calls and being consistent with the rest of the method, but there may be something beyond the scope of my problems that made you go with that approach, I do not know. Thank you for looking at it anyways, I will try to see if there's something else at my end.
        Hide
        Richard A. Sitze added a comment -

        Now I'm curious as to the behaviour you are seeing. At the following code
        point:

        // Fourth, try the fallback implementation class
        if (factory == null)

        { factory = newFactory(FACTORY_DEFAULT, LogFactory.class.getClassLoader()); }

        Yes, LogFactory.class.getClassLoader() may return null. The method newFactory
        checks the class loaders, sees that it is null, and drops down to the line:

        return (LogFactory)Class.forName(factoryClass).newInstance();

        which is exactly the desired behavior (Class is LogFactory.class). If,
        instead, you pass in 'contextClassLoader', then you allow the user to override
        the factory-default by providing another implementation (same name) down the
        classloader hierarchy. This is 'one-way' to resolve the problem, but the
        programming model is to use other methods to override... You shouldn't NEED
        the context classloader, as LogFactory and the factory default are (suppose to
        be) in the same jar file. If you don't have the factory default (commons-
        logging-api.jar?), then this is moot anyway.

        Does that help any, or am I still blind?

        Show
        Richard A. Sitze added a comment - Now I'm curious as to the behaviour you are seeing. At the following code point: // Fourth, try the fallback implementation class if (factory == null) { factory = newFactory(FACTORY_DEFAULT, LogFactory.class.getClassLoader()); } Yes, LogFactory.class.getClassLoader() may return null. The method newFactory checks the class loaders, sees that it is null, and drops down to the line: return (LogFactory)Class.forName(factoryClass).newInstance(); which is exactly the desired behavior (Class is LogFactory.class). If, instead, you pass in 'contextClassLoader', then you allow the user to override the factory-default by providing another implementation (same name) down the classloader hierarchy. This is 'one-way' to resolve the problem, but the programming model is to use other methods to override... You shouldn't NEED the context classloader, as LogFactory and the factory default are (suppose to be) in the same jar file. If you don't have the factory default (commons- logging-api.jar?), then this is moot anyway. Does that help any, or am I still blind?
        Hide
        Philip Mitchell added a comment -

        okay, getCachedFactory() was fixed in the new source, although I copied my
        original post right out of the source code view (right out of the cvs
        repository), but I didn't see that updated code until just now.

        No matter, the bigger problem (and yet more subtle) that I had in mind is still
        there in getFactory():

        // Fourth, try the fallback implementation class
        if (factory == null)

        { factory = newFactory(FACTORY_DEFAULT, LogFactory.class.getClassLoader()); }

        but LogFactory.class.getClassLoader() returns null. I've suggested (and tested
        on my system) replacing 'LogFactory.class.getClassLoader()'
        with 'contextClassLoader' since that was defined earlier and used in every
        other place in the method.

        This problem doesn't occur in standalone apps or in a lot of other cases
        because it doesn't get to this fallback implementation class, but in my case it
        gets there.

        Show
        Philip Mitchell added a comment - okay, getCachedFactory() was fixed in the new source, although I copied my original post right out of the source code view (right out of the cvs repository), but I didn't see that updated code until just now. No matter, the bigger problem (and yet more subtle) that I had in mind is still there in getFactory(): // Fourth, try the fallback implementation class if (factory == null) { factory = newFactory(FACTORY_DEFAULT, LogFactory.class.getClassLoader()); } but LogFactory.class.getClassLoader() returns null. I've suggested (and tested on my system) replacing 'LogFactory.class.getClassLoader()' with 'contextClassLoader' since that was defined earlier and used in every other place in the method. This problem doesn't occur in standalone apps or in a lot of other cases because it doesn't get to this fallback implementation class, but in my case it gets there.
        Hide
        Richard A. Sitze added a comment -

        OK, I've reviewed the code, and obviously I'm missing something
        because it looks good to me (and I'm not having problems). You
        have done some homework to pin-point the problem... would you
        please help me out with a reference to where you think the problem
        is occuring in the new code... [btw, this is subtle - at least
        I find it so]

        Again, the code below is "old", so if you seeing a two if statements
        in getCachedFactory() then you have out-of-date code. I'm confused...

        Show
        Richard A. Sitze added a comment - OK, I've reviewed the code, and obviously I'm missing something because it looks good to me (and I'm not having problems). You have done some homework to pin-point the problem... would you please help me out with a reference to where you think the problem is occuring in the new code... [btw, this is subtle - at least I find it so] Again, the code below is "old", so if you seeing a two if statements in getCachedFactory() then you have out-of-date code. I'm confused...
        Hide
        Philip Mitchell added a comment -

        Please note: the previous note was not intended to sound confrontational. I
        just wanted to point out that I already (before I posted the bug originally)
        had tested with the most recent build to begin with and whatever fix had been
        applied for this problem wasn't quite complete. I apologise if I've offended
        anyone, I should have chosen my words more carefully.
        Thank you for reconsidering.

        Show
        Philip Mitchell added a comment - Please note: the previous note was not intended to sound confrontational. I just wanted to point out that I already (before I posted the bug originally) had tested with the most recent build to begin with and whatever fix had been applied for this problem wasn't quite complete. I apologise if I've offended anyone, I should have chosen my words more carefully. Thank you for reconsidering.
        Hide
        Philip Mitchell added a comment -

        Then how do you explain the error I get when I'm using the MOST RECENT BUILD.
        LogFactory.class.getClassLoader() still returns null and it is not handled
        well, yet LogFactory.getContextClassLoader() does not. I don't know what
        you've put into it to "fix" this, but it's not quite right yet.
        Please reconsider looking at this, because it obviously isn't fully resolved.

        Show
        Philip Mitchell added a comment - Then how do you explain the error I get when I'm using the MOST RECENT BUILD. LogFactory.class.getClassLoader() still returns null and it is not handled well, yet LogFactory.getContextClassLoader() does not. I don't know what you've put into it to "fix" this, but it's not quite right yet. Please reconsider looking at this, because it obviously isn't fully resolved.
        Hide
        Richard A. Sitze added a comment -

        Please try recent code from CVS (a recent nightly build will do).
        The code you are looking at was changed on 8/29/02. Many other
        changes have gone in to correct the problem you describe.

            • This bug has been marked as a duplicate of 10825 ***
        Show
        Richard A. Sitze added a comment - Please try recent code from CVS (a recent nightly build will do). The code you are looking at was changed on 8/29/02. Many other changes have gone in to correct the problem you describe. This bug has been marked as a duplicate of 10825 ***
        Hide
        Philip Mitchell added a comment -

        correction: changing the first part to include a call to getContextClassLoader
        () does not quite do it, changing the 'if(factory = null)' to an 'else' will
        clear it up. This may not be what was intended, but I don't think it will
        change the function of the code significantly. Please consider this fix.
        Thanks

        Show
        Philip Mitchell added a comment - correction: changing the first part to include a call to getContextClassLoader () does not quite do it, changing the 'if(factory = null)' to an 'else' will clear it up. This may not be what was intended, but I don't think it will change the function of the code significantly. Please consider this fix. Thanks

          People

          • Assignee:
            Unassigned
            Reporter:
            Philip Mitchell
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development