RFR: 8129988: JSSE should create a single instance of the cacerts KeyStore

[Xuelei Fan]

On 9/30/2015 9:44 PM, Sean Mullan wrote:
> On 9/29/15 10:33 PM, Xuelei Fan wrote:
>> private synchronized static KeyStore getDefaultCacertsKeyStore()
>> ================================================================
>>
>> ---------------
>> private synchronized static KeyStore getDefaultCacertsKeyStore(
>>                              String javaHome, String type,
>>                              String provider, String password,
>>                              String dbgOption) throws Exception {
>>      if (defaultCacertsKeyStore != null) {
>>          return defaultCacertsKeyStore;
>>      }
>> ---------------
>>
>> Looks like the parameters would impact the returned value.  However, as
>> defaultCacertsKeyStore is singleton instance, these parameters have no
>> impact any more after the initialization.  This is a behavior change.
>> The behavior change is OK to me, but we maybe want to add a comment or
>> twist the method a little bit.
> 
> This is true, but I don't think this is really a legitimate use case.
> First, changing the system properties while an app is running is not
> going to work very well in a multithreaded environment. Second, even in
> a single threaded case, I can't think of a valid use case where you
> would want to use the jssecacerts/cacerts file (i.e.
> javax.net.ssl.trustStore is not set) and subsequently change the values
> of the keystore type, provider or password system properties while the
> app was running. Note that this behavior is still supported as before if
> you do set the javax.net.ssl.trustStore property, and the properties are
> read and used each time to create a new KeyStore instance.
> 
> I agree a comment would be helpful though, so I will add that. If you
> think we should also release note this, let me know.
> 
I agree the impact should be very limited in practice, may not worthy a
CCC request or release note.  Add a comment should be sufficient in case
code readers get confused.

>> For lazy static fields, I would like to use "Lazy initialization holder
>> class idiom" [#71, Effective Java, 2ED], rather than synchronized
>> method, for performance benefits.
> 
> Good point, although I am a bit wary of using that pattern in this case
> because it can throw an Exception. See "Only One Chance To Initialize"
> at https://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom
> 
Exceptions can be swallowed for "null" value, I think.

Xuelei

> --Sean
> 
>>
>> Thanks,
>> Xuelei
>>
>> On 9/30/2015 2:07 AM, Sean Mullan wrote:
>>> Please review this fix to modify the TrustManagerFactory implementation
>>> to create a single instance of the cacerts or jssecacerts KeyStore. This
>>> significantly improves performance in a multithreaded environment.
>>>
>>> The code has been refactored a bit to move common code into a few
>>> private methods.
>>>
>>> webrev: http://cr.openjdk.java.net/~mullan/webrevs/8129988/webrev.00/
>>>
>>> Thanks,
>>> Sean
>>