GROOVY-12303: ClassNodeResolver: NoClassDefFoundError during class-loader lookup aborts resolution - #2834
GROOVY-12303: ClassNodeResolver: NoClassDefFoundError during class-loader lookup aborts resolution#2834daniellansun wants to merge 4 commits into
Conversation
…ader lookup aborts resolution
fcba553 to
00f7e31
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2834 +/- ##
==================================================
+ Coverage 70.6486% 70.6972% +0.0486%
- Complexity 36522 36564 +42
==================================================
Files 1571 1571
Lines 133963 133994 +31
Branches 24690 24692 +2
==================================================
+ Hits 94643 94730 +87
+ Misses 30802 30765 -37
+ Partials 8518 8499 -19
🚀 New features to boost your workflow:
|
ClassNodeResolver.isSourceNewer (and GroovyClassLoader.isSourceNewer) opened non-file source URLs with the default URLConnection cache, so a jar: lookup left the JAR mapped. On Windows that prevented JUnit from deleting the @tempdir used by ClassNodeResolverTest.nonFileSourceUrlUsesUrlConnectionForFreshness.
URLStreams.getLastModified is the single place for file: File.lastModified (including the historical Windows | drive-letter form) and uncached non-file URL connections. ClassNodeResolver and GroovyClassLoader isSourceNewer keep their own timestamp and interval contracts.
…tion NCDFE recovery decompiled the same resource more than once and then called tryAsScript with no oldClass, which skips isSourceNewer and the same-loader check. Parse once; replace with a script only when the class came from another loader; rethrow when there is no matching bytecode.
|
✅ All tests passed ✅🏷️ Commit: 8114f54 Learn more about TestLens at testlens.app/docs. |
blackdrag
left a comment
blackdrag
left a comment
There was a problem hiding this comment.
I think I am seeing a conceptual problem in this code. And the problem may have existed already after the asm mode had been added. What I am missing is a clear definition.
| ASM | ClassLoader | |
|---|---|---|
| normal | on | on |
| ASM-only | on | off |
| loader-only | off | on |
| neither | off | off |
For each row in that table we have to define what is allowed and what should be done in general, when to fall back to script and what to do in case of a class loading error or an asm format error. When is it allowed to load a class, even though we are in asm-only mode, or when it is allowed to decompile even though we are in loader-only mode. The current code is mixing everything together. And if we do our work here on differing assumptions we will both invest a lot of work without getting a result. If some of those are impossible, then the question is where that gets established.
But I have two points about the code...
- it makes no sense adding a test for a simple constructor. We need tests on findClassNode instead of the structures it uses. Maybe tests that enter even higher. In other words there is too much whitebox testing. Plus, the tests that are there, are not testing the different modes, they test only the default mode.
- about the script being more new than the class. If we assume that the loader can contain the class we are actually looking for then a check for if the parent knows that by loading or knows the class as resource is actually not good enough. In case of a NCDFE we no longer have the class, and therefore we cannot perform the existing cls.getClassLoader() != loader check to establish that the class is really from the parent. The fact that a class can be found through the parent is not quite equivalent to knowing that the failed class was defined by the parent. This is of course largely because GroovyClassLoader does not behave like a standard class loader.
3 participants
https://issues.apache.org/jira/browse/GROOVY-12303