Pivot
  1. Pivot
  2. PIVOT-687

BXMLSerializer, would like to be able to specify a classloader for loading custom components

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0.1
    • Component/s: core-beans
    • Labels:
      None
    • Environment:
      all

      Description

      I'm running into issues trying to load my pivot window into a swing application. Some background,
      since pivot 2.0 can load components into a swing application, I've been trying to integrate pivot into a netbeans platform application.
      Only been at it a couple of hours but I'm stuck on an class loader issue. Basically, because I have pivot wrapped in separate module it's classloader
      can't references classes in any modules that depend on it. See here:
      http://bits.netbeans.org/dev/javadoc/org-openide-modules/org/openide/modules/doc-files/classpath.html#loader-hier

      I looked briefly in the docs but I'm not sure if I missed a readObject() signature that will allow me to pass in the classloader to use when de-serializing.

      1. patch2.patch
        3 kB
        Sandro Martini
      2. patch.patch
        1 kB
        Sandro Martini

        Issue Links

          Activity

          Hide
          Sandro Martini added a comment -

          In an multi-layered application, a good thing would be to have a way for a caller to pass its (or maybe even a custom) Classloader instance to BXMLSerializer, so anytime a dynamic load of a class is needed, when that classloader is not null BXMLSerializer can use it instead of the default one (good for all standard cases).

          Examples of Classloaders (for callers, to pass):
          // ClassLoader classLoader = ClassLoader.getSystemClassLoader(); // clSystem
          // ClassLoader classLoader = this.getClass().getClassLoader(); // clCurrent
          // ClassLoader classLoader = Thread.currentThread().getContextClassLoader(); // clContext
          Currently BXMLSerializer by default uses this:
          ClassLoader classLoader = Thread.currentThread().getContextClassLoader();

          Note that probably this is a good feature even in other places inside Pivot, whenever a dynamic load of a class has to be done, but this could be a later evolution (if needed here).
          For example, I had a dedicated class for this, with the following methods (extract):
          public DynamicLoader(String className, String variantName, int namingStrategy, boolean exceptionOnNull)
          that was much more complex than needed here because I had to handle different naming strategies for class names.
          In my custom bean factory OptionalVariantBeanFactory i was loading classes using using DynamicLoader, and before loading the requested class I tried to load the optional variant version (specific for the given customer code) and if not found try to load the default class.
          Probably all this is too business-specific for inclusion in Pivot, but a simplified version of DynamicLoader could be useful (maybe in the future).

          protected transient boolean initClassToLoad = true;
          protected transient ClassLoader classloader = null;

          public void useClassloaderEnhanced(boolean initClass, ClassLoader cl)
          this was using to enable the "enhanced" classloading, so later (inside the real load method) I could use
          if (classloader != null)
          clazz = Class.forName(fullClassName, initClassToLoad, classloader);
          else
          clazz = Class.forName(fullClassName);

          just as a sample ...

          Show
          Sandro Martini added a comment - In an multi-layered application, a good thing would be to have a way for a caller to pass its (or maybe even a custom) Classloader instance to BXMLSerializer, so anytime a dynamic load of a class is needed, when that classloader is not null BXMLSerializer can use it instead of the default one (good for all standard cases). Examples of Classloaders (for callers, to pass): // ClassLoader classLoader = ClassLoader.getSystemClassLoader(); // clSystem // ClassLoader classLoader = this.getClass().getClassLoader(); // clCurrent // ClassLoader classLoader = Thread.currentThread().getContextClassLoader(); // clContext Currently BXMLSerializer by default uses this: ClassLoader classLoader = Thread.currentThread().getContextClassLoader(); Note that probably this is a good feature even in other places inside Pivot, whenever a dynamic load of a class has to be done, but this could be a later evolution (if needed here). For example, I had a dedicated class for this, with the following methods (extract): public DynamicLoader(String className, String variantName, int namingStrategy, boolean exceptionOnNull) that was much more complex than needed here because I had to handle different naming strategies for class names. In my custom bean factory OptionalVariantBeanFactory i was loading classes using using DynamicLoader, and before loading the requested class I tried to load the optional variant version (specific for the given customer code) and if not found try to load the default class. Probably all this is too business-specific for inclusion in Pivot, but a simplified version of DynamicLoader could be useful (maybe in the future). protected transient boolean initClassToLoad = true; protected transient ClassLoader classloader = null; public void useClassloaderEnhanced(boolean initClass, ClassLoader cl) this was using to enable the "enhanced" classloading, so later (inside the real load method) I could use if (classloader != null) clazz = Class.forName(fullClassName, initClassToLoad, classloader); else clazz = Class.forName(fullClassName); just as a sample ...
          Hide
          GBivins added a comment - - edited

          More info here. I tried modifying BXMLSerializer to use the other signature for Class.forName.
          Since the default behavior of the current call is to use true with whatever is the current loader

          http://download.oracle.com/javase/1.4.2/docs/api/java/lang/Class.html#forName(java.lang.String)

          I did the following:
          changing the calls:
          ...~ln 737
          propertyClass = Class.forName(propertyClassName);
          ...~ln 754
          Class<?> type = Class.forName(className);

          in the processStartElement method of BXMLSerializer to use the signature that includes the classloader:

          propertyClass = Class.forName(propertyClassName,true,Thread.currentThread().getContextClassLoader());
          Class<?> type = Class.forName(className,true,Thread.currentThread().getContextClassLoader());

          This did the trick for me. Since I'm in NBP, there is a system class loader which is (by default, ie can be overriden) the same as the context classloader for any thread in the NB vm.
          See here for more details:
          http://bits.netbeans.org/dev/javadoc/org-openide-modules/org/openide/modules/doc-files/classpath.html#syscl

          Show
          GBivins added a comment - - edited More info here. I tried modifying BXMLSerializer to use the other signature for Class.forName. Since the default behavior of the current call is to use true with whatever is the current loader http://download.oracle.com/javase/1.4.2/docs/api/java/lang/Class.html#forName(java.lang.String ) I did the following: changing the calls: ...~ln 737 propertyClass = Class.forName(propertyClassName); ...~ln 754 Class<?> type = Class.forName(className); in the processStartElement method of BXMLSerializer to use the signature that includes the classloader: propertyClass = Class.forName(propertyClassName,true,Thread.currentThread().getContextClassLoader()); Class<?> type = Class.forName(className,true,Thread.currentThread().getContextClassLoader()); This did the trick for me. Since I'm in NBP, there is a system class loader which is (by default, ie can be overriden) the same as the context classloader for any thread in the NB vm. See here for more details: http://bits.netbeans.org/dev/javadoc/org-openide-modules/org/openide/modules/doc-files/classpath.html#syscl
          Hide
          Sandro Martini added a comment - - edited

          first (minimal) version, just to fix the (wrong) behavior.

          In there aren't objections, I can commit this (now, or max before the end of the week), and later make a more general fix so I can pass here a custom Classloader too (post here a patch2 before applying it).

          Show
          Sandro Martini added a comment - - edited first (minimal) version, just to fix the (wrong) behavior. In there aren't objections, I can commit this (now, or max before the end of the week), and later make a more general fix so I can pass here a custom Classloader too (post here a patch2 before applying it).
          Hide
          Greg Brown added a comment -

          What testing did you do to verify this fix?

          Show
          Greg Brown added a comment - What testing did you do to verify this fix?
          Hide
          Sandro Martini added a comment -

          Hi Greg, at the moment only few tests from Standalone Applications, like those done by GBivins time ago ... maybe we could try to contact him and have some more feedback.

          For sure I have to make other tests (inside Applets, and more important inside Java Web Start because here we could find some problem). I won't commit until those tests are successfully passed.
          And then ask to other users to update to trunk and retry on their applications.

          Show
          Sandro Martini added a comment - Hi Greg, at the moment only few tests from Standalone Applications, like those done by GBivins time ago ... maybe we could try to contact him and have some more feedback. For sure I have to make other tests (inside Applets, and more important inside Java Web Start because here we could find some problem). I won't commit until those tests are successfully passed. And then ask to other users to update to trunk and retry on their applications.
          Hide
          Sandro Martini added a comment -

          After this (first version), I'm thinking to use an approach like mine (described earlier, used in production some year ago) ... like in BXMLSerializer, add a get/setClassloader(), and if not set (default null) the behavior is like today (pre-first fix), otherwise using the given Classloader. Ok ?

          And last, I've found uses in other Pivot sources of the Java forName() method to dynamically load classes, so maybe we can think to wrap the dynamic load (and related optional Classloader), as a dedicated Class, and use it where needed (like here) ... comments ?

          Bye,
          Sandro

          Show
          Sandro Martini added a comment - After this (first version), I'm thinking to use an approach like mine (described earlier, used in production some year ago) ... like in BXMLSerializer, add a get/setClassloader(), and if not set (default null) the behavior is like today (pre-first fix), otherwise using the given Classloader. Ok ? And last, I've found uses in other Pivot sources of the Java forName() method to dynamically load classes, so maybe we can think to wrap the dynamic load (and related optional Classloader), as a dedicated Class, and use it where needed (like here) ... comments ? Bye, Sandro
          Hide
          Greg Brown added a comment -

          It may make sense to provide a ClassLoader constructor argument to BXMLSerializer.

          Show
          Greg Brown added a comment - It may make sense to provide a ClassLoader constructor argument to BXMLSerializer.
          Hide
          Sandro Martini added a comment -

          Ok, my proposal of a setter was to avoid to change existing constructors, but I like this way too .

          Show
          Sandro Martini added a comment - Ok, my proposal of a setter was to avoid to change existing constructors, but I like this way too .
          Hide
          Sandro Martini added a comment -

          This is the patch for a more general solution:
          add a constructor with a ClassLoader as parameter, used in any dynamic load here if not null, otherwise the old behavior is used.

          Show
          Sandro Martini added a comment - This is the patch for a more general solution: add a constructor with a ClassLoader as parameter, used in any dynamic load here if not null, otherwise the old behavior is used.
          Hide
          Sandro Martini added a comment -

          Note that patch2 now makes obsolete the previous patch.

          I've done some tests with the Kitchen Sink demo: using the old (zero argument constructor), and using the new but passing null, or a ClassLoader created in the usual 3 ways describer at the start of this ticket, and in all cases all works good.

          Tell me if something in not good, or without objections next week I'll commit this fix.
          I'll try to contact the original reporter for this ticket, and see if this fix is good even for him.

          Show
          Sandro Martini added a comment - Note that patch2 now makes obsolete the previous patch. I've done some tests with the Kitchen Sink demo: using the old (zero argument constructor), and using the new but passing null, or a ClassLoader created in the usual 3 ways describer at the start of this ticket, and in all cases all works good. Tell me if something in not good, or without objections next week I'll commit this fix. I'll try to contact the original reporter for this ticket, and see if this fix is good even for him.
          Hide
          GBivins added a comment -

          Sorry guys, I've been pretty busy @ work but I have been watching this. I'll try to test out the patch this weekend and let you know how it goes.

          Show
          GBivins added a comment - Sorry guys, I've been pretty busy @ work but I have been watching this. I'll try to test out the patch this weekend and let you know how it goes.
          Hide
          Sandro Martini added a comment -

          Just committed the fix. In case of other classloader issues, please reopen it and reassign to 2.0.1 and to me. Thank you.

          Bye,
          Sandro

          Show
          Sandro Martini added a comment - Just committed the fix. In case of other classloader issues, please reopen it and reassign to 2.0.1 and to me. Thank you. Bye, Sandro
          Hide
          Sandro Martini added a comment -

          Reference to the discussion:
          http://apache-pivot-users.399431.n3.nabble.com/BXMLSerializer-can-I-specify-a-classloader-td2101779.html

          Note that now there is even this issue ( https://issues.apache.org/jira/browse/PIVOT-742 ) that could re-open this ticket, so I think we need a solution that leave both ways working.

          Show
          Sandro Martini added a comment - Reference to the discussion: http://apache-pivot-users.399431.n3.nabble.com/BXMLSerializer-can-I-specify-a-classloader-td2101779.html Note that now there is even this issue ( https://issues.apache.org/jira/browse/PIVOT-742 ) that could re-open this ticket, so I think we need a solution that leave both ways working.
          Hide
          Sandro Martini added a comment -

          The fix for PIVOT-742 probably break this.
          We have to see better how to solve it ...
          Generally speaking I like the idea to pass a Classloader to a factory (to use in corner cases) ... we'll see.
          After I'll recontact GBivins and ask for other tests.

          Show
          Sandro Martini added a comment - The fix for PIVOT-742 probably break this. We have to see better how to solve it ... Generally speaking I like the idea to pass a Classloader to a factory (to use in corner cases) ... we'll see. After I'll recontact GBivins and ask for other tests.
          Hide
          Andrei Pozolotin added a comment -

          @Sandro:

          in my opinion, the use of both BXMLSerializer(classLoader) & TCCL is major incosistency;
          (which resulted in bug addressed by patch_2011-06-15_bxml-classloader.patch)
          since all of pivot-wtk uses TCCL, it sort of dictates that pivot-core must use TCCL as well.

          @GBivins:

          can you please confirm that "TCCL bracket" (below) is a working replacement for "BXMLSerializer(classLoader)":

          @Override
          public final void run() {
          final Thread thread = Thread.currentThread();
          final ClassLoader pastLoader = thread.getContextClassLoader();
          final ClassLoader thisLoader = getClass().getClassLoader();
          try

          { thread.setContextClassLoader(thisLoader); /* INIT YOUR BXML HERE; BOTH PIVOT-CORE & PIVOT-WTK WILL USE thisLoader NOW */ }

          catch (Throwable e)

          { log.error("", e); }

          finally

          { thread.setContextClassLoader(pastLoader); }

          }

          Show
          Andrei Pozolotin added a comment - @Sandro: in my opinion, the use of both BXMLSerializer(classLoader) & TCCL is major incosistency; (which resulted in bug addressed by patch_2011-06-15_bxml-classloader.patch) since all of pivot-wtk uses TCCL, it sort of dictates that pivot-core must use TCCL as well. @GBivins: can you please confirm that "TCCL bracket" (below) is a working replacement for "BXMLSerializer(classLoader)": @Override public final void run() { final Thread thread = Thread.currentThread(); final ClassLoader pastLoader = thread.getContextClassLoader(); final ClassLoader thisLoader = getClass().getClassLoader(); try { thread.setContextClassLoader(thisLoader); /* INIT YOUR BXML HERE; BOTH PIVOT-CORE & PIVOT-WTK WILL USE thisLoader NOW */ } catch (Throwable e) { log.error("", e); } finally { thread.setContextClassLoader(pastLoader); } }
          Hide
          GBivins added a comment - - edited

          Hi guys, my initial tests with the trunk code seem to work as I originally expected...By that I mean, because of the way netbeans classloading works:

          ( http://bits.netbeans.org/dev/javadoc/org-openide-modules/org/openide/modules/doc-files/classpath.html#loader-hier)

          I don't need any special handling of setting the classloader to load bxml and referenced resources.
          If I understand the changes to how BXMLSerializer works now, it is looking to load resources on the current threads context classloader.
          In a netbeans app, that is the expected behavoir (see link below for details):
          http://bits.netbeans.org/dev/javadoc/org-openide-modules/org/openide/modules/doc-files/classpath.html#syscl

          I'll will run some more tests later this afternoon but so far, the current fixes seem to be the way to go, at least from the NBP side of things!

          Show
          GBivins added a comment - - edited Hi guys, my initial tests with the trunk code seem to work as I originally expected...By that I mean, because of the way netbeans classloading works: ( http://bits.netbeans.org/dev/javadoc/org-openide-modules/org/openide/modules/doc-files/classpath.html#loader-hier ) I don't need any special handling of setting the classloader to load bxml and referenced resources. If I understand the changes to how BXMLSerializer works now, it is looking to load resources on the current threads context classloader. In a netbeans app, that is the expected behavoir (see link below for details): http://bits.netbeans.org/dev/javadoc/org-openide-modules/org/openide/modules/doc-files/classpath.html#syscl I'll will run some more tests later this afternoon but so far, the current fixes seem to be the way to go, at least from the NBP side of things!
          Hide
          Andrei Pozolotin added a comment -

          @Gerrick:

          yes, TCCL is "official" classloader in NetBeans RCP;
          later I suggest we replace TCCL with "pluggable classloader" PIVOT-742,
          but default implementaion to be shipped with pivot will still be TCCL; so you will not get affected

          thank you for your tests!

          Andrei.

          Show
          Andrei Pozolotin added a comment - @Gerrick: yes, TCCL is "official" classloader in NetBeans RCP; later I suggest we replace TCCL with "pluggable classloader" PIVOT-742 , but default implementaion to be shipped with pivot will still be TCCL; so you will not get affected thank you for your tests! Andrei.
          Hide
          Sandro Martini added a comment -

          Hi all, thank you very much for your new tests (and comments).
          Be free to reopen this issue if something will happen.

          Bye

          Show
          Sandro Martini added a comment - Hi all, thank you very much for your new tests (and comments). Be free to reopen this issue if something will happen. Bye

            People

            • Assignee:
              Sandro Martini
              Reporter:
              GBivins
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development