On Tue, 16 Mar 2021 15:29:51 GMT, Xue-Lei Andrew Fan <xue...@openjdk.org> wrote:

>> Doesn't that still incur a JVM-wide lock whenever 
>> `CachedSecureRandomHolder.getDefSecureRandom()` is called?  I recall a 
>> different JVM-wide lock in the `getInstance` path 
>> ([JDK-7107615](https://bugs.openjdk.java.net/browse/JDK-7107615)) creating 
>> significant performance issues.
>
> I did not get the point that the code path should be unaffected". As there 
> are a few update like:
>          init(keysize, JCAUtil.getSecureRandom());
>          init(keysize, JCAUtil.getDefSecureRandom());
> 
> I think there are some impact on the existing application which use the 
> default sec
> 
>> Or, I can also separate out the instance returned by getSecureRandom() so 
>> that code path should be unaffected, e.g.:
> 
> I did not get the point that the "code path should be unaffected"? 
> JCAUtil.getDefSecureRandom() is used in updates like:
>          init(keysize, JCAUtil.getSecureRandom());
>          init(keysize, JCAUtil.getDefSecureRandom());
> 
>> 
>> ```
>>     // cached SecureRandom instance
>>     private static class CachedSecureRandomHolder {
>>         public static SecureRandom instance = new SecureRandom();
>>         private static Provider[] cachedConfig = Security.getProviders();
>>         private static SecureRandom def = instance;
>> 
>>         public static SecureRandom getDefault() {
>>             synchronized (CachedSecureRandomHolder.class) {
>>                 Provider[] currConfig = Security.getProviders();
>>                 if (!Arrays.equals(cachedConfig, currConfig)) {
>>                     def = new SecureRandom();
>>                     cachedConfig = currConfig;
>>                 }
>>             }
>>             return def;
>>         }
>>     }
>> 
>>     /**
>>      * Get a SecureRandom instance. This method should be used by JDK
>>      * internal code in favor of calling "new SecureRandom()". That needs to
>>      * iterate through the provider table to find the default SecureRandom
>>      * implementation, which is fairly inefficient.
>>      */
>>     public static SecureRandom getSecureRandom() {
>>         return CachedSecureRandomHolder.instance;
>>     }
>> 
>>     /**
>>      * Get the default SecureRandom instance. This method is the
>>      * optimized version of "new SecureRandom()" which re-uses the default
>>      * SecureRandom impl if the provider table is the same.
>>      */
>>     public static SecureRandom getDefSecureRandom() {
>>         return CachedSecureRandomHolder.getDefault();
>>     }
>> ```

> As this behavior has been a while, I may not update the implementation. 
> Instead, I would like to add an "implNote" tag to state that once the default 
> provider is loaded, an implementation may be changed it any longer. 
> Applications could use provider parameter if there is a preference.

Well, javadoc of the affected init methods clearly specifies that "get them 
using the SecureRandom implementation of the highest-priority installed 
provider as the source of randomness. ", thus I am not sure if we can just get 
around this with an _@implNote_.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3018

Reply via email to