Details

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

      Description

      openjpa uses since some versions asm but in tomee for instance we use a shade (through the xbean one org.apache.xbean:xbean-asm-shaded to avoid classloading/version issues with webapps/applications.

      it can be nice if openjpa can provide us an issue to this because we really don't want to provide asm in our binaries.

      solutions can be to use the xbean shade, give us an interface to implement or to try several implementation (xbean shade, asm,...) by reflection

      1. ASM.patch
        6 kB
        Romain Manni-Bucau
      2. kernel.patch
        9 kB
        Rick Curtis

        Issue Links

          Activity

          Hide
          Kevin Sutter added a comment -

          Can you help us understand the basic issue so that we can resolve this appropriately? Is the basic issue because OpenJPA has introduced a maven dependency on ASM 3.2? Or, is it due to the fact that we have now included the asm binaries in our openjpa-all jar file? And, are those asm binaries causing conflicts with asm usage by TomEE? I looked at the maven shade plugin (http://maven.apache.org/plugins/maven-shade-plugin/), but I'm wondering how that would affect those OpenJPA users that are okay with our current setup. For example, other OpenJPA users could be using our jar file that does not contain all of the dependencies and just substitute their own copies (for example, the consumer is already using asm 3.2, so we're good to go). Do users of the shade plugin end up providing both shaded and non-shaded versions of the jar files? And, why is ASM the concern? Wouldn't this same concern apply to other open-source dependencies that are specified (apache commons, derby, etc)?

          Just looking for some more background and justification for this type of change. Thanks.

          Show
          Kevin Sutter added a comment - Can you help us understand the basic issue so that we can resolve this appropriately? Is the basic issue because OpenJPA has introduced a maven dependency on ASM 3.2? Or, is it due to the fact that we have now included the asm binaries in our openjpa-all jar file? And, are those asm binaries causing conflicts with asm usage by TomEE? I looked at the maven shade plugin ( http://maven.apache.org/plugins/maven-shade-plugin/ ), but I'm wondering how that would affect those OpenJPA users that are okay with our current setup. For example, other OpenJPA users could be using our jar file that does not contain all of the dependencies and just substitute their own copies (for example, the consumer is already using asm 3.2, so we're good to go). Do users of the shade plugin end up providing both shaded and non-shaded versions of the jar files? And, why is ASM the concern? Wouldn't this same concern apply to other open-source dependencies that are specified (apache commons, derby, etc)? Just looking for some more background and justification for this type of change. Thanks.
          Hide
          Romain Manni-Bucau added a comment -

          the issue is mainly tomee uses it in common classloader of tomcat and any user can need to provide a different version of asm (for a spring webapp for instance). as this issue is common Geronimo shaded asm.

          the goal of this issue is to simply the dependency of openjpa on objectweb asm library.

          if a user depends on asm it will need to bring asm.

          Maybe i was not clear but the idea is simply to replace a dependency by another one for openjpa so the classical dependencies mecanisms are not broken.

          Show
          Romain Manni-Bucau added a comment - the issue is mainly tomee uses it in common classloader of tomcat and any user can need to provide a different version of asm (for a spring webapp for instance). as this issue is common Geronimo shaded asm. the goal of this issue is to simply the dependency of openjpa on objectweb asm library. if a user depends on asm it will need to bring asm. Maybe i was not clear but the idea is simply to replace a dependency by another one for openjpa so the classical dependencies mecanisms are not broken.
          Show
          Romain Manni-Bucau added a comment - FYI: patched openejb/tomee version: http://svn.apache.org/repos/asf/openejb/trunk/patched-libraries/openjpa-all-asm-shaded/
          Hide
          Jean-Louis MONTEIRO added a comment -

          Well, IMHO the issue is larger than just TomEE. The point is that a lot of projects uses ASM (Hibernate, OpenJPA, Spring, CXF, etc ...). But almost all of them uses a different version which can lead to hard classloading issues to solve. I can remember pulling my hair out because of that.

          Even out TomEE, a webapp can face the same issue.

          If you could provide an ASM-shaded jar of OpenJPA, that could help solving such an issue.

          wdyt?

          Show
          Jean-Louis MONTEIRO added a comment - Well, IMHO the issue is larger than just TomEE. The point is that a lot of projects uses ASM (Hibernate, OpenJPA, Spring, CXF, etc ...). But almost all of them uses a different version which can lead to hard classloading issues to solve. I can remember pulling my hair out because of that. Even out TomEE, a webapp can face the same issue. If you could provide an ASM-shaded jar of OpenJPA, that could help solving such an issue. wdyt?
          Hide
          Mark Struberg added a comment -

          I'll try to take a look at this today.

          Show
          Mark Struberg added a comment - I'll try to take a look at this today.
          Hide
          Romain Manni-Bucau added a comment -

          a patch using reflection to be able to use asm, xbean-asm or spring-asm

          Show
          Romain Manni-Bucau added a comment - a patch using reflection to be able to use asm, xbean-asm or spring-asm
          Hide
          Mark Struberg added a comment -

          slightly tweaked the patch and applied it to trunk

          Thanks to rmannibucau for providing the fix!

          Show
          Mark Struberg added a comment - slightly tweaked the patch and applied it to trunk Thanks to rmannibucau for providing the fix!
          Hide
          Rick Curtis added a comment -

          Hey guys, question about this one. I was doing some testing with Java 7 + ASM 4.0 and found that this change negatively affected my scenario. I believe that Kevin also mentioned this in OPENJPA-2283, but I don't know if you guys had the time to look through that info. The net of it is that if I change my source level from 1.6 -> 1.7 and the asm dependency to 4.0 I get the same exception as documented in 2283. If I revert AsmAdapter back to pre 1241719, everything works fine.

          I think this is something that should get addressed prior to cutting a 2.3.0 release.

          Show
          Rick Curtis added a comment - Hey guys, question about this one. I was doing some testing with Java 7 + ASM 4.0 and found that this change negatively affected my scenario. I believe that Kevin also mentioned this in OPENJPA-2283 , but I don't know if you guys had the time to look through that info. The net of it is that if I change my source level from 1.6 -> 1.7 and the asm dependency to 4.0 I get the same exception as documented in 2283. If I revert AsmAdapter back to pre 1241719, everything works fine. I think this is something that should get addressed prior to cutting a 2.3.0 release.
          Hide
          ASF subversion and git services added a comment -

          Commit 1530808 from Mark Struberg in branch 'openjpa/branches/2.3.x'
          [ https://svn.apache.org/r1530808 ]

          OPENJPA-2283 use xbean-asm4-shaded ASM version as the dynamic handling doesn't work out

          This makes sure we always have a guaranteed ASM version 4 regardless what ASM a
          user might add to the project. This also rolls back the dynamic ASM handling of
          OPENJPA-2171.

          Show
          ASF subversion and git services added a comment - Commit 1530808 from Mark Struberg in branch 'openjpa/branches/2.3.x' [ https://svn.apache.org/r1530808 ] OPENJPA-2283 use xbean-asm4-shaded ASM version as the dynamic handling doesn't work out This makes sure we always have a guaranteed ASM version 4 regardless what ASM a user might add to the project. This also rolls back the dynamic ASM handling of OPENJPA-2171 .
          Hide
          Rick Curtis added a comment -

          Hey Mark, re your previous commit... I know Tomee uses the xbean dependency, but that isn't true for all users of OpenJPA (ie: WebSphere). Could we do something smarter where we have a compile time dependency on plain asm AND xbean-asm... then at runtime we can load whichever implementation is available? I'm thinking something along the lines of this patch.

          Show
          Rick Curtis added a comment - Hey Mark, re your previous commit... I know Tomee uses the xbean dependency, but that isn't true for all users of OpenJPA (ie: WebSphere). Could we do something smarter where we have a compile time dependency on plain asm AND xbean-asm... then at runtime we can load whichever implementation is available? I'm thinking something along the lines of this patch.
          Hide
          Romain Manni-Bucau added a comment -

          @Rick: I think it is the case, if xbean shade is availabe let use it cause we control it otherwise just use what is available. The patch was just about the order as far as i understood.

          Show
          Romain Manni-Bucau added a comment - @Rick: I think it is the case, if xbean shade is availabe let use it cause we control it otherwise just use what is available. The patch was just about the order as far as i understood.
          Hide
          Rick Curtis added a comment -

          Mark's latest commit reverted the magic involved with picking whatever implementation is available... and now has a hard runtime dependency on xbean-asm.

          Show
          Rick Curtis added a comment - Mark's latest commit reverted the magic involved with picking whatever implementation is available... and now has a hard runtime dependency on xbean-asm.
          Hide
          Romain Manni-Bucau added a comment -

          outch, sorry missed it. We should keep it even if trying asm4 xbean shade first for spring and cxf users at least.

          Show
          Romain Manni-Bucau added a comment - outch, sorry missed it. We should keep it even if trying asm4 xbean shade first for spring and cxf users at least.
          Hide
          Rick Curtis added a comment -

          I'm just pointing out that we need to be careful with what we're doing. Not all users of OpenJPA want to use the xbean-asm implementation.

          Show
          Rick Curtis added a comment - I'm just pointing out that we need to be careful with what we're doing. Not all users of OpenJPA want to use the xbean-asm implementation.
          Hide
          Mark Struberg added a comment -

          Rick, I fear there are not many options.
          ASM is broken in regards to classloading and we had to fix this with overriding a very method.
          And this can only be done via reflection if you create a subclass on the fly (using byte code enhancement).
          This would be a chicken-egg problem and imo not worth the hassle.

          Please note that we do not make things worse: previously there has been a hardcoded dependency to ASM native where we do not know anything about the version, etc. It would be even possible that some projects would not use maven and then you have 2 different ASM libs on the classpath which will get you in deep troubles.

          By using our own well known shaded version from our very own xbean project (xbean is an Apache Geronimo sub project), we have at least a guarantee that we do not get into those problems. At the same time we avoid classpath clashes with other ASM versions by strictly using an own shaded package name (org.apache.xbean.asm4).

          The dynamic stuff was worth a try, but it turned out that a few bugs in ASM prevent us from doing so.

          Show
          Mark Struberg added a comment - Rick, I fear there are not many options. ASM is broken in regards to classloading and we had to fix this with overriding a very method. And this can only be done via reflection if you create a subclass on the fly (using byte code enhancement). This would be a chicken-egg problem and imo not worth the hassle. Please note that we do not make things worse: previously there has been a hardcoded dependency to ASM native where we do not know anything about the version, etc. It would be even possible that some projects would not use maven and then you have 2 different ASM libs on the classpath which will get you in deep troubles. By using our own well known shaded version from our very own xbean project (xbean is an Apache Geronimo sub project), we have at least a guarantee that we do not get into those problems. At the same time we avoid classpath clashes with other ASM versions by strictly using an own shaded package name (org.apache.xbean.asm4). The dynamic stuff was worth a try, but it turned out that a few bugs in ASM prevent us from doing so.
          Hide
          Rick Curtis added a comment -

          Mark –

          Kevin and I talked this afternoon and I think we can swallow the xbean dependency in 2.3.x going forward. I need to think some more on it, but I think we're good to go for now.

          Show
          Rick Curtis added a comment - Mark – Kevin and I talked this afternoon and I think we can swallow the xbean dependency in 2.3.x going forward. I need to think some more on it, but I think we're good to go for now.
          Hide
          Mark Struberg added a comment - - edited

          great, txs! I
          I think it helps to know that xbean-asm4-shaded is nothing else than the standard ASM jar but with it's packages shaded to a unique package name per major version (as those contain binary backward incompatibilities). It does not have any runtime dependency. Not to a geronimo project nor to anything else.

          Show
          Mark Struberg added a comment - - edited great, txs! I I think it helps to know that xbean-asm4-shaded is nothing else than the standard ASM jar but with it's packages shaded to a unique package name per major version (as those contain binary backward incompatibilities). It does not have any runtime dependency. Not to a geronimo project nor to anything else.
          Hide
          Rick Curtis added a comment -

          Mark Struberg – I'd like to see the new java.version property broke apart into something like java.class.version and java.testclass.version. This allows us to build OpenJPA with one version and run tests with another.

          Show
          Rick Curtis added a comment - Mark Struberg – I'd like to see the new java.version property broke apart into something like java.class.version and java.testclass.version. This allows us to build OpenJPA with one version and run tests with another.
          Hide
          Mark Struberg added a comment -

          We can do this of course, and fill it with the java.version by default. But I'm not quite sure what the benefit would be. For doing our release we will still use java6 for some time. And for running all the build in our CI the single property is fine enough. I have no objection to introduce it though - just like to verify if it's really useful before we have another property to maintain

          Show
          Mark Struberg added a comment - We can do this of course, and fill it with the java.version by default. But I'm not quite sure what the benefit would be. For doing our release we will still use java6 for some time. And for running all the build in our CI the single property is fine enough. I have no objection to introduce it though - just like to verify if it's really useful before we have another property to maintain
          Hide
          Kevin Sutter added a comment -

          Hi Mark,
          Since Java 8 is just around the corner, we will have customers attempting to use Java 8 with their Entity classes. And, they are bound to have issues... So, it's nice to allow our testcases to be built with a different class version than what OpenJPA runtime is built with. We had originally put this in due to the Java 6 / Java 7 requirement. We might as well leave it for the Java 7 / Java 8 requirement. Thanks.

          Show
          Kevin Sutter added a comment - Hi Mark, Since Java 8 is just around the corner, we will have customers attempting to use Java 8 with their Entity classes. And, they are bound to have issues... So, it's nice to allow our testcases to be built with a different class version than what OpenJPA runtime is built with. We had originally put this in due to the Java 6 / Java 7 requirement. We might as well leave it for the Java 7 / Java 8 requirement. Thanks.
          Hide
          Mark Struberg added a comment -

          we finally settled on switching to xbean-asm4-shaded. This jar shades all the asm4 stuff into an own package org.apache.xbean.asm4 to reassure that we do not have any nasty classpath clashes.

          The properties to tweak the target JVM version have been implemented as well.

          Show
          Mark Struberg added a comment - we finally settled on switching to xbean-asm4-shaded. This jar shades all the asm4 stuff into an own package org.apache.xbean.asm4 to reassure that we do not have any nasty classpath clashes. The properties to tweak the target JVM version have been implemented as well.

            People

            • Assignee:
              Mark Struberg
              Reporter:
              Romain Manni-Bucau
            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development