[CRYPTO-169][FOLLOWUP] Fix reentrancy of CryptoRandomFactory#getCryptoRandom
#259
Conversation
|
It seems that there are some differences between Java 8 and 17, let me take another look. |
🆗 |
Codecov Report
@@ Coverage Diff @@
## master #259 +/- ##
============================================
+ Coverage 74.66% 74.83% +0.16%
- Complexity 441 444 +3
============================================
Files 38 38
Lines 1816 1820 +4
Branches 177 178 +1
============================================
+ Hits 1356 1362 +6
+ Misses 349 348 -1
+ Partials 111 110 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -206,6 +206,18 @@ public static CryptoRandom getCryptoRandom(final Properties props) | |||
| } else { | |||
| throw initializerError; | |||
| } | |||
| } catch (final NoClassDefFoundError noClassDefFoundError) { | |||
| Throwable initializerError = noClassDefFoundError.getCause(); | |||
There was a problem hiding this comment.
There are some differences here between Java 8/11 and Java 17/21:
- Java 8/11: Will catch
NoClassDefFoundError, butNoClassDefFoundError.getCauseisnull. - Java 17/21: Will catch
NoClassDefFoundError, andNoClassDefFoundError.getCauseisExceptionInInitializerError, butExceptionInInitializerError.getCauseis null
So the check way here is a bit tricky, do you have any better suggestions? @garydgregory
There was a problem hiding this comment.
@LuciferYang
Not off the top of my head...
LuciferYang
force-pushed
the
CRYPTO-169-FOLLOWUP
branch
from
934011e to
69adb01
Compare
|
|
||
| if (set.contains(name)) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Add a collection to record classes that have ExceptionInInitializerError to avoid unnecessary reentry of the Class.forName method in this scenario. This collection cannot be garbage collected, otherwise there will still be reentry issues.
| @@ -63,6 +68,9 @@ private static abstract class AbstractNegativeCacheSentinel { | |||
| public static Class<?> getClassByName(final String name) throws ClassNotFoundException { | |||
| final Class<?> ret = getClassByNameOrNull(name); | |||
| if (ret == null) { | |||
| if (INIT_ERROR_CLASSES.get(CLASSLOADER).contains(name)) { | |||
| throw new IllegalStateException("Class " + name + " initialization error"); | |||
There was a problem hiding this comment.
An additional judgment has been added to distinguish ClassNotFoundException. Here an IllegalStateException is manually thrown, there may be a more appropriate exception type.
| @@ -198,14 +198,6 @@ public static CryptoRandom getCryptoRandom(final Properties props) | |||
| } catch (final Exception e) { | |||
| lastException = e; | |||
| errorMessage.append("CryptoRandom: [" + className + "] failed with " + e.getMessage()); | |||
| } catch (final ExceptionInInitializerError initializerError) { | |||
There was a problem hiding this comment.
ExceptionInInitializerError is uniformly handled in ReflectionUtils, and an exception is thrown at the same time. So, there is no need to handle it here.
|
Thanks @garydgregory ~ |
YW! |
There are some issues with the reentrancy of
CryptoRandomFactory.getCryptoRandom(properties)method.For the following test case:
The second call to
CryptoRandomFactory.getCryptoRandom(properties)will result in the following error:This is due to the
Class.forNamemethod. If the first initialization fails, subsequent identical calls will not attempt to initialize the class again, but will throwNoClassDefFoundError.So this PR changes to add a collection in
ReflectionUtilsto record the class names that failed to initialize (recorded whenClass.forNamefirst fails to initialize), to ensure that known failed initialization classes will not be attempted to load repeatedly.This PR removes the modification of the
CryptoRandomFactory#getCryptoRandommethod in #258, because under the above changes,CryptoRandomFactory#getCryptoRandomwill no longer catch ExceptionInInitializerError.