On Mon, 15 Mar 2021 21:31:12 GMT, Anthony Scarpino <ascarp...@openjdk.org> 
wrote:

>> It depends, this may not be a "hot area" where there is a lot of contention? 
>> Or do you feel maybe we should just go with the slower "new SecureRandom()" 
>> call for each affected class? I am on the fence actually.
>
> Since 5 classes have init() calls that would be acquiring this lock, 
> something that could be independent would be better.  Calling "new 
> SecureRandom()" may or may not be better, but hope it wouldn't lock over 
> operations during creation.
> 
> I'm just throwing out an idea:
> Can the provider index number be verify in the list each time?
> 
> Better yet, maybe a way to have this triggered each time a provider is added 
> or removed from the list?

Cipher and KeyPairGenerator could be used a lot in TLS context.  The 
synchronization may impact in some circumstances.  Supporting dynamic 
configuration could be error-prone.  For example, one thread to set the 
providers, and another thread get the providers by calling Cipher.init().  
Unless the set and get use the same lock, the return value of the get method is 
not predictable.  The lock here is on CachedSecureRandomHolder for providers 
configuration getting, and no lock (or different lock) for providers 
configuration setting.  So applications may be able to get the expected 
behaviors by introducing this synchronization.

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.

If you want go ahead, maybe the performance impacted could be mitigated by 
locking less processes.  For example:

if (checkConfig) { 
    Provider[] currConfig = Security.getProviders();
    if (!Arrays.equals(cachedConfig, currConfig)) {
       synchronized (CachedSecureRandomHolder.class) {
           // double check the currConfig
           currConfig = Security.getProviders();
          if (!Arrays.equals(cachedConfig, currConfig)) {
              instance = ...
              cachedConfig = ...
           }
       }
    }
}
return instance;

However, I still not very sure of the performance of Security.getProviders() 
because it is synchronized as well.  Further, I may want to evaluate if the 
update of Security provider (for example Providers.setProviderList) could 
trigger an even to update the default instances.

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

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

Reply via email to