Commons JXPath
  1. Commons JXPath
  2. JXPATH-127

Change dynamic class loading to consult context class loader.

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.4
    • Labels:
      None

      Description

      For dynamically loading classes, JXPath currently uses Class.forName(...). This means all classes loaded by JXPath must be visible to whatever class loader loaded JXPath. This is restrictive in large frameworks and web applications where multiple class loaders are in use. In particular, in an EJB3 application deployed on Weblogic, the shared jars between different EJB modules are loaded with a separate class loader than the modules themselves. Consequently, if the jxpath.jar is among the general libraries for the application, then a specific EJB module will be unable to reference static methods of classes within its own module in an xpath expression. For example, consider the following lines of code in an EJB module class:

      JXPathContext context = JXPathContext.newContext(new Object());
      Object value = context.getValue("SomeClass.getSomething()");

      This will fail, with a ClassNotFoundException, if SomeClass is a class in the module, but jxpath is a general library for the application.

      Attached (or will be once I figure out how) is a patch for JXPath that gets around this issue. The patch changes the class loading strategy to use the context class loader, when it appears appropriate, to dynamically load classes. The strategy is based on suggestions and example code from:

      http://www.javaworld.com/javaworld/javaqa/2003-06/01-qa-0606-load.html

      Assuming all class loaders involved delegate to their parent class loader (as recommended in the API), currently working code, that uses JXPath, should only be affected by this change if the current class loader (the class loader that loaded JXPath) is not in an ancestor/descendant relationship with the context class loader.

        Activity

        Hide
        Matt Benson added a comment -

        After much delay...

        Committed revision 916559.

        Show
        Matt Benson added a comment - After much delay... Committed revision 916559.
        Hide
        John Trimble added a comment -

        I grant the ASF use of the parts of patch jxpath-14-classloading-lang-port-with-fallback.patch that were written by me.

        I also added yet another patch (sorry) which is the same except that it includes the Apache License header on the source files for the added test case.

        Show
        John Trimble added a comment - I grant the ASF use of the parts of patch jxpath-14-classloading-lang-port-with-fallback.patch that were written by me. I also added yet another patch (sorry) which is the same except that it includes the Apache License header on the source files for the added test case.
        Hide
        John Trimble added a comment -

        The same as jxpath-14-classloading-lang-port-with-fallback.patch, but with the missing ASF license source header on the added test classes.

        Show
        John Trimble added a comment - The same as jxpath-14-classloading-lang-port-with-fallback.patch, but with the missing ASF license source header on the added test classes.
        Hide
        Matt Benson added a comment - - edited

        Using [lang] code is not a problem as it is already ASL licensed code either owned by or granted use to the ASF. I think I'm okay with including the last patch. Can you comment back indicating that you grant the ASF use of the parts of this patch written by you?

        Show
        Matt Benson added a comment - - edited Using [lang] code is not a problem as it is already ASL licensed code either owned by or granted use to the ASF. I think I'm okay with including the last patch. Can you comment back indicating that you grant the ASF use of the parts of this patch written by you?
        Hide
        John Trimble added a comment -

        Thank you for taking the time to respond to my post. I understand and appreciate your concern regarding the first patch I attached, and I apologize for the trouble. I looked over Roubtsov's code and the only thing taken from him code-wise is the use of a dummy SecurityManager to get the call stack. This isn't necessary so I removed it and uploaded a new patch. Of course, the patch still uses the same approach to class loading as described in the article (which may be the root of your IP concern). If you still feel there are IP issues, I can contact Roubtsov about an IP grant. However, I've included 2 additional patches, based on your suggestion of using [lang]'s code, one of which might work better anyways.

        The patch jxpath-14-classloading-lang-port.patch is, for the most part, a verbatim port of [lang]'s class loading code. I made some modifications to remove dependence on StringUtil. It also includes a test case. My concern with using [lang]'s class loading code is that, while it solves the original problem, it may cause backwards compatibility issues. Specifically, anyone currently using [jxpath] in an application where the context class loader is set will now have jxpath attempting to load their classes through the context class loader (which may not have access to them) instead of the class loader that loaded jxpath as it used to (the jxpath-14-classloading-widest-scope.patch patch solves this problem by choosing the class loader with the widest scope).

        The patch jxpath-14-classloading-lang-port-with-fallback.patch is also a port of [lang]'s code but modified to use the current class loader (the class loader that loaded ClassLoaderUtil) if the context class loader throws a ClassNotFoundException. This should make the class loading change to [jxpath] transparent to those already using it, and essentially achieves the same goal as the jxpath-14-classloading-widest-scope.patch patch. So either patch jxpath-14-classloading-widest-scope.patch or jxpath-14-classloading-lang-port-with-fallback.patch would be best, in my opinion, but any of them should solve the problem. I have not marked any of them for inclusion due to the IP concerns with the first and the other two being ports of code, not written by me, from [lang]. I can change that, but I wanted to err on the side of caution this time. Let me know what you think and thanks again.

        Show
        John Trimble added a comment - Thank you for taking the time to respond to my post. I understand and appreciate your concern regarding the first patch I attached, and I apologize for the trouble. I looked over Roubtsov's code and the only thing taken from him code-wise is the use of a dummy SecurityManager to get the call stack. This isn't necessary so I removed it and uploaded a new patch. Of course, the patch still uses the same approach to class loading as described in the article (which may be the root of your IP concern). If you still feel there are IP issues, I can contact Roubtsov about an IP grant. However, I've included 2 additional patches, based on your suggestion of using [lang] 's code, one of which might work better anyways. The patch jxpath-14-classloading-lang-port.patch is, for the most part, a verbatim port of [lang] 's class loading code. I made some modifications to remove dependence on StringUtil. It also includes a test case. My concern with using [lang] 's class loading code is that, while it solves the original problem, it may cause backwards compatibility issues. Specifically, anyone currently using [jxpath] in an application where the context class loader is set will now have jxpath attempting to load their classes through the context class loader (which may not have access to them) instead of the class loader that loaded jxpath as it used to (the jxpath-14-classloading-widest-scope.patch patch solves this problem by choosing the class loader with the widest scope). The patch jxpath-14-classloading-lang-port-with-fallback.patch is also a port of [lang] 's code but modified to use the current class loader (the class loader that loaded ClassLoaderUtil) if the context class loader throws a ClassNotFoundException. This should make the class loading change to [jxpath] transparent to those already using it, and essentially achieves the same goal as the jxpath-14-classloading-widest-scope.patch patch. So either patch jxpath-14-classloading-widest-scope.patch or jxpath-14-classloading-lang-port-with-fallback.patch would be best, in my opinion, but any of them should solve the problem. I have not marked any of them for inclusion due to the IP concerns with the first and the other two being ports of code, not written by me, from [lang] . I can change that, but I wanted to err on the side of caution this time. Let me know what you think and thanks again.
        Hide
        John Trimble added a comment -

        Port of [lang]'s class loading code. Code is modified to fall back on the current class loader if the context class loader throws a ClassNotFoundException.

        Show
        John Trimble added a comment - Port of [lang] 's class loading code. Code is modified to fall back on the current class loader if the context class loader throws a ClassNotFoundException.
        Hide
        John Trimble added a comment -

        A port of [lang]'s class loading code with some modifications to avoid pulling in unnecessary classes from [lang].

        Show
        John Trimble added a comment - A port of [lang] 's class loading code with some modifications to avoid pulling in unnecessary classes from [lang] .
        Hide
        John Trimble added a comment -

        A modification of the classloading_fix_jxpath14.patch patch. Does not use a dummy SecurityManager to get the call stack.

        Show
        John Trimble added a comment - A modification of the classloading_fix_jxpath14.patch patch. Does not use a dummy SecurityManager to get the call stack.
        Hide
        Matt Benson added a comment - - edited

        I am as yet undecided on this approach though I am amenable due to the fact that [lang]'s ClassUtils#getClass(...) family of methods default to the current Thread context loader in a similar manner. That said, the code you've borrowed from the article in question is marked as copyrighted (I like the way you've granted the ASF permission to use someone else's code, btw) so there would have to be a formal IP grant before [jxpath] could use this patch as-is. It would be simpler to copy [lang]'s code and use that within [jxpath].

        Show
        Matt Benson added a comment - - edited I am as yet undecided on this approach though I am amenable due to the fact that [lang] 's ClassUtils#getClass(...) family of methods default to the current Thread context loader in a similar manner. That said, the code you've borrowed from the article in question is marked as copyrighted (I like the way you've granted the ASF permission to use someone else's code, btw) so there would have to be a formal IP grant before [jxpath] could use this patch as-is. It would be simpler to copy [lang] 's code and use that within [jxpath] .
        Hide
        John Trimble added a comment -

        Patch for the trunk of JXPath: http://svn.apache.org/repos/asf/commons/proper/jxpath/trunk

        Changes class loading strategy in JXPath to consult context class loader.

        Show
        John Trimble added a comment - Patch for the trunk of JXPath: http://svn.apache.org/repos/asf/commons/proper/jxpath/trunk Changes class loading strategy in JXPath to consult context class loader.

          People

          • Assignee:
            Unassigned
            Reporter:
            John Trimble
          • Votes:
            2 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development