Support ASM7_EXPERIMENTAL if system property is set

[ijuma]

Full support for Java 11 bytecode requires ASM7_EXPERIMENTAL. The
approach used here is similar to byte-buddy's:

https://github.com/raphw/byte-buddy/blob/30c31ab403eedce386e28acc3e6aee2a7e2e0752/byte-buddy-dep/src/main/java/net/bytebuddy/utility/OpenedClassReader.java#L42

[ijuma]

@sameb Would an approach like this one be considered? If so, I can add some unit tests.

[sameb]

how far off is a stable asm7? regardless, i think adding the ASM_API constant is a good idea, so if you want to submit a patch that just changes all the ASM6 refs to ASM_API and hard-code that to ASM6 for now, that'd be great. then we can followup with updating what the ASM_API refers to.

[raphw]

LGTM. ASM 7 is probably not released before late September when Java 11 is released.

[ijuma]

I am happy to split this into two PRs. The argument for having an experimental mode (like byte-buddy) is that cgllib is used by testing libraries, build tools and widely used frameworks. Assuming that ASM sticks to their current model where the "experimental" tag is only removed after the respective Java version ships, people can only test their project with unreleased Java versions if cglib supports something like this. And if projects can't test against unreleased Java versions, bugs are only found too late.

This is going to be a recurring issue with the new Java release cadence.

[sameb]

I have some quibbles about the system property name, but the concept sounds fine to me. Happy to resolve naming issues in this PR or in a separate smaller one that just redoes the constant.

Naming concerns: "experimental" doesn't really seem to explain what this is doing. A lot of things could be experimental. Perhaps "experimantal_asm" or "experimental_asm7"? I can't imagine folks successfully keeping the var set over time, and we're going to have to ship new cglib anyway to update the constant, so including the asm version seems useful. WDYT?

[ijuma]

Yes, I also thought that having a property for the specific ASM version may reflect what it does better. Assuming that there is no other use for experimental functionality.

[ijuma]

PR that just makes the refactoring: #131

I'll update this one after that is merged.

[ijuma]

Thanks for merging #131. Rebased, updated the system property and added a unit test.

[ijuma]

I verified that EasyMock's tests pass with ASM7_EXPERIMENTAL enabled.

[sameb]

Could you update the PR to cache the value so it doesn't have to reread the system properties every time? That may make tests harder, but it's worth it I think.

[ijuma]

@sameb we effectively do that via the Constants class, right?

[sameb]

Oh! Right! Sorry, my bad for misreading. Thanks for clarifying.

[ijuma]

Thanks for the quick review!

[ijuma]

@sameb any chance of getting a release with this change? Thanks.

[sameb]

I'm on vacation for 2 weeks (w/o a computer with a terminal). Will make a release when I get back.

…

On Fri, Aug 17, 2018, 8:57 PM Ismael Juma ***@***.***> wrote: @sameb <https://github.com/sameb> any chance of getting a release with this change? Thanks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#130 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACC7EoxnAtKgDLoQuXY8Fdaz82CfXjoZks5uR2ZvgaJpZM4Vve0G> .

[ijuma]

Thanks for replying quickly (while on vacation) and for the upcoming release! Enjoy your vacation. :)

[ijuma]

@sameb Are you still planning to do a release soon? Thanks!

[sameb]

Yup, though I'm testing out a fix for a JDK11 quirk first. Turns out that JDK11 started writing bridge methods more frequently than before, which can mess up users who use Enhancer.getMethods & have interceptors that prefer matching non-bridge methods (to avoid accidentally triggering twice when bridging across signatures because of erasure). This is the cause of the failure @ google/guice#1199 (comment) -- the test wants to match non-bridge methods, but cglib is only returning bridge methods when compiled w/ JDK11. The fix is to explicitly avoid bridge methods in subclasses when the superclass has a non-bridge method with the exact same signature. I've written up the fix & am testing it across our corpus to ensure it works, and will commit & release after things are good.

[ijuma]

Great, thanks for the update. Do you know if this change in behaviour is intentional? If not, it may be worth reporting it.

[sameb]

already asked those exact questions, and received confirmation that the changes were intentional & related to the following bugs / JEPS: https://bugs.openjdk.java.net/browse/JDK-8203488, https://bugs.openjdk.java.net/browse/JDK-8133247, http://openjdk.java.net/jeps/181

[ijuma]

Perfect, thanks for the information!

[sameb]

FYI, just to give an update, my first pass at the fix didn't work -- there were some edge cases it failed on. I'm trying another approach now and will commit if it works.

[sameb]

released version 3.2.8, should show up on maven central in a couple of hours

[ijuma]

Thanks!