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