Core: Handle ExceptionInInitializerError & NoClassDefFoundError invoke-time failures in DynMethods callers#16611
Core: Handle ExceptionInInitializerError & NoClassDefFoundError invoke-time failures in DynMethods callers#16611nastra wants to merge 1 commit into
Conversation
nastra
changed the title
nastra
force-pushed
the
dyn-handling
branch
2 times, most recently
from
4490a7f to
e773f63
Compare
| Class<?> targetClass = classForName(className); | ||
| impl(targetClass, types); | ||
| } catch (NoClassDefFoundError | ClassNotFoundException e) { | ||
| } catch (NoClassDefFoundError | ClassNotFoundException | ExceptionInInitializerError e) { |
There was a problem hiding this comment.
Please remove changes to this class. I don't think that this is necessary to fix the bug and this is an over-broad change to the contract of our reflection classes that are used in a lot of places.
rdblue
left a comment
rdblue
left a comment
There was a problem hiding this comment.
This needs to be much more narrow. We can talk about changes to the reflection classes separately.
nastra
force-pushed
the
dyn-handling
branch
2 times, most recently
from
2ffb413 to
bcb4854
Compare
nastra
changed the title
| } | ||
|
|
||
| @Test | ||
| void registerDoesNotFailWhenClassThrowsNoClassDefFoundOnInvoke() { |
There was a problem hiding this comment.
These tests are a bit awkward, and hard to understand what they are actually test. Shall we create a package private classesToRegister() method and override it in the tests?
There was a problem hiding this comment.
reproducing the original issue in a unit test without mocking out a bunch of things is a bit difficult because we'd have to simulate that parquet/orc isn't on the classpath. Also I didn't want to provide a fix without having a test that at least shows how the issue can happen in practice. So I figured we might want to just have a try/catch block that essentially does what's being handled in the affected classes.
I'm open to other ideas though. Using a package-private classesToRegister() might work for FormatModelRegistry but not for InternalData.
There was a problem hiding this comment.
What if we create a 'register(String)' method both in InternalData and FormatModelRegistry?
We can call this from the prouction code and from the test too.
…e-time failures in DynMethods callers
4 participants
DynMethodsuseClass.forNamewhich can throwNoClassDefFoundErrororExceptionInInitializerErrorwhen a method is found but its bodyreferences a missing transitive dependency or triggers a failing static
initializer. This causes
FormatModelRegistry.<clinit>andInternalData.<clinit>to failpermanently when optional modules like
iceberg-orcoriceberg-parquetare absent (#16720).This widens the catch blocks in
FormatModelRegistry.registerSupportedFormats()and
InternalData.registerSupportedFormats()to handle these invoke-timefailures the same way as
NoSuchMethodExceptionThe initial version was created by Claude and tests were manually adjusted