Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] ASM & Lombok while running Tests #3474

Closed
BjoernAkAManf opened this issue Aug 3, 2023 · 15 comments
Closed

[BUG] ASM & Lombok while running Tests #3474

BjoernAkAManf opened this issue Aug 3, 2023 · 15 comments
Milestone

Comments

@BjoernAkAManf
Copy link

Describe the bug
Trying to test an application with both ASM and lombok fails.

"Potential easy solution" (TM): Rename the lombok shaded ASM package, so other software does not fail.

To Reproduce
Trying to use ASM and lombok at the same time will fail when running Tests (using IntelliJ or Maven).

As far as i can see everything is as described by https://projectlombok.org/setup/maven.
Note that running the main Method works, however running jUnit will fail.

I created a minimal reproduceable example.

The error basically says that ASM is used by both lombok and ASM.
java.lang.LayerInstantiationException: Package org.objectweb.asm in both module lombok and module org.objectweb.asm

Expected behavior
Testing works out of the box and the Project compiles.

Version info (please complete the following information):

  • Lombok version 1.18.28
  • Platform maven 3.9.2 and IntelliJ IntelliJ IDEA 2023.2 (Ultimate Edition) Build #IU-232.8660.185, built on July 26, 2023

Additional context
See #2549 and #2973

PS: Thanks for the great software! I love using lombok, but it's quite a shame the issue above occurs. The previous listed issues boil down to:

  1. Use explicit Annotation Processor (i specified them)
  2. Require static lombok (i do)
  3. No minimal example provided (see above)
  4. User Error (I don't see how i'm doing something wrong, and i don't think JUnit / Maven is doing something explicitely wrong. Especially given, that lombok can be quite helpful generating Test Data Classes, where records do not suffice.

I'd be willing to give a try implementing reshading, but understanding the code base has been quite hard for me in the past.
So any pointer, if that change would be welcome, would be appreciated.

@BjoernAkAManf BjoernAkAManf changed the title [BUG] [BUG] ASM & Lombok while running Tests Aug 3, 2023
@rspilker
Copy link
Collaborator

rspilker commented Aug 3, 2023

We're working on a solution ourselves.

@rzwitserloot
Copy link
Collaborator

Hey @BjoernAkAManf - thanks for spotting this one. We've taken a different approach to the usual shader solutions (because the usual solutions are IMO rather bad). We published an edge release with this fix; can you test if it fixes the problem? Thank you!

@rzwitserloot rzwitserloot modified the milestones: next-version, v1.18.28 Aug 4, 2023
@BjoernAkAManf
Copy link
Author

BjoernAkAManf commented Aug 4, 2023

Hi guys!

I don't think this fix works as intended though. Tested it in both IntelliJ and through maven.

https://github.com/BjoernAkAManf/mre-lombok-asm/actions/runs/5760113052

EDIT: Also checked the Changelog includes the changes linked above.

@Rawi01
Copy link
Collaborator

Rawi01 commented Aug 5, 2023

The change also breaks classloading in eclipse...

@BjoernAkAManf
Copy link
Author

@rzwitserloot could you reopen it? I think it got forgotten.

@rspilker
Copy link
Collaborator

It is included and part of the Edge Release.

@BjoernAkAManf
Copy link
Author

It is included and part of the Edge Release.

Hi @rzwitserloot i do not think that the issue is resolved, given my base mre is still failing.

See https://github.com/BjoernAkAManf/mre-lombok-asm/actions/runs/6205806866/job/16849373259#step:4:507

Error Message:

Corrupted channel by directly writing to native stream in forked JVM 1. Stream 'java.lang.LayerInstantiationException: Package org.objectweb.asm in both module lombok and module org.objectweb.asm'.

@rzwitserloot
Copy link
Collaborator

@BjoernAkAManf Thanks for testing it. I'm.. not sure what's left for us to do at this point. The code we wrote works and org.objectweb is neither loaded nor present as something that anything else could possibly load. What you report is as far as I can tell not possible.

Clearly it is, but the problem is, I now have absolutely no idea where to take it from here. We'd need a self-contained test case that reliably triggers this problem without requiring us to download 5GB onto our drives and download a few book worth of knowledge on whatever project setup you have which might as this point be a challenge.

Are you sure you're using the edge release in all places it matters?

@BjoernAkAManf
Copy link
Author

Hi @rzwitserloot the example linked above contains the following line in apache maven (distribution of which is 10mb).

2023-09-16T06:29:07.1030722Z [INFO] Downloading from projectlombok.org: https://projectlombok.org/edge-releases/org/projectlombok/lombok/edge-SNAPSHOT/lombok-edge-20230915.025955-1.jar
2023-09-16T06:29:07.6357314Z [INFO] Downloaded from projectlombok.org: https://projectlombok.org/edge-releases/org/projectlombok/lombok/edge-SNAPSHOT/lombok-edge-20230915.025955-1.jar (2.0 MB at 2.6 MB/s)

The pom only defines the lombok repository (plus the central repository implicitly).

Dependencies are only to JUnit, lombok and asm.
The git repository is only 8 or so files long each (i'd assume less than 10 mb each) and might be cloned shallow.

Maven, Ant and Gradle are not that diffferent, so i'd assume that is not the issue.

So as far as can see it's more or less git clone && mwnw verify to reproduce. I do not see where this reaches a file size of more than 1 GB let alone 5.

Anything else i can help you with? I could probably create a shell example with hard coded jars as well, would that be more helpful?

@Rawi01
Copy link
Collaborator

Rawi01 commented Sep 17, 2023

I'm suprised that this error occurs at runtime because lombok should not be at the classpath at that point. It is a compile time only dependency and the fix only affects the compilation steps. Can you try to exclude it for the surefire tests?

@rzwitserloot
Copy link
Collaborator

@BjoernAkAManf No need for shell scripts. Sounds like your example repo here will do fine as test case. Reopening.

@rzwitserloot rzwitserloot reopened this Sep 17, 2023
@rzwitserloot
Copy link
Collaborator

rzwitserloot commented Sep 19, 2023

EDIT: Our conclusions in this comment are wrong - see next comment for explanation of what's really going on here!

Cloned the project.

  • Yes, it fails.
  • Doing actual tests, it just isn't us. I think surefire perhaps ships with lombok somehow? i.e. an older version?
  • mvn (the surefire plugin, rather) starts surefire with an @ file for the args, and it deletes that file instantly. It is just not possible to debug this stuff, and that's on surefire, not on us. Thus, we're throwing in the towel: We're not fixing it, maybe surefire can fix it for you / can at least make the debug process less onerous. All that's needed for us to proceed is that we can actually see what it is doing, so we can check what modules it is actually throwing to shove on the module path, which is currently not possible no matter how many -X or -e args you provide to maven.
  • If you use the <configuration> property in the surefire section of your pom file with: <useModulePath>false</useModulePath>, all works well again. That should be a convenient workaround until you can tell surefire to fix this / fix their launch procedure so there's a way to tell it not to use @ files, or echo them to the command line on an -X / -e run, or to NOT delete the args file instantly after using it.

@rzwitserloot rzwitserloot closed this as not planned Won't fix, can't repro, duplicate, stale Sep 19, 2023
@rzwitserloot rzwitserloot reopened this Sep 19, 2023
@rzwitserloot
Copy link
Collaborator

rzwitserloot commented Sep 19, 2023

It's the dejavu ticket that bounces between closed and reopened.

Sooo, what's the programmer's version of treppenwitz? Where you toss in the towel, close the tabs and the relevant editors, and go do something else, and at that exact moment you go: Oh waaaaait a second?

Anyway, that.

Conclusion: Team OpenJDK is... ugh. They make bizarro and dangerous choices a lot.

The wait a second moment is this: The JVM figures out this split package thing really quickly - like, we tell surefire to freeze on boot so we can maybe see that @ file or connect to it and check some things and we never even get that far. We ask the JVM to print every class as it loads it, and it never prints anything relevant (zero lombok classes, for example). So, clearly then, it opens those modules and realizes there's a split package situation before it possibly could which is.. well, not possible.

Unless....

The java runtime just opens the jar and scans directories. If it sees 2 directories that match it immediately goes: Oh, oh! Split packages, abort abort. Which is silly. Maybe wait until something is loaded from there?

At any rate, that's it, that's why it still fails. It doesn't matter that we never load anything in the namespace, or that the concept of 'a package name' is a notion that exists only on the 'other side' of the classloader line (not on the 'go find the sources' line, it's on the defineClass / returns from getResource side of the line), so OpenJDK jumps the gun and this is a bug on their end. Which I'm sure they're neither going to spec nor fix, but, fortunately, we can fix it easily.

We've now updated our shadowloader to work off of a subdir instead. I cannot reiterate how disappointingly bad this is from how the JDK loads modules. Just to confirm, adding a 0-length file to the jars with path org/objectweb/asm/oh-dear.txt is enough to get surefire plugin to asplode again. Proving that this is it.

Fix will make it in time for the imminent 1.18.30 release. It'll be closed, for the last time, in an hour or two while I go clean up the commits and write a note in the changelog for it.

@BjoernAkAManf
Copy link
Author

While I agree mostly with the sentiment, i think the requirement, that module directories are reserved to the actual module, so that classes and resources of a package only exist in their own Jar. It is not as intuitive as it should be. Especially with that error.

However, I'm glad the issue is fixed. I'll update the Repository as fixed once i get around to it. =)

@mjustin
Copy link

mjustin commented Nov 10, 2023

Sooo, what's the programmer's version of treppenwitz? Where you toss in the towel, close the tabs and the relevant editors, and go do something else, and at that exact moment you go: Oh waaaaait a second?

It's a new term (apparently circa 2009), but maybe "shower thought"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants