On Mon, 15 Mar 2021 21:36:08 GMT, Xue-Lei Andrew Fan <xue...@openjdk.org> wrote:
>> 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. I would definitely not call `new SecureRandom()` each time as that can have real performance problems (especially on systems which pull from `/dev/random` and may not have hardware entropy). The synchronization worries me too. Could `java.security.Security` contain a "provider version" `int` (volatile, atomic, or similar) which is incremented every time the provider list is changed? Then this class (clearly using some non-public API) would check that value and if it is the same, just return the cached instance. If it has changed since the last time, then it would update `instance` and its remembered value of the index. I think that we might be able to do it without taking any locks in this class because even if the providers change out from under us, we returned a reasonable value (subject to multi-threading) and will fix things up when we're called the next time. // using this and name collision to highlight what's a field and what's local int providerListVersion = Security.nonPublicGetProviderListVersion(); if (this.providerListVersion != providerListVersion) { this.providerListVersion = providerListVersion; this.instance = new SecureRandom(); } return this.instance; ------------- PR: https://git.openjdk.java.net/jdk/pull/3018