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

[Sean Mullan]

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.

> 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

--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
>