RFR 7004967: SecureRandom should be more explicit about threading

[Wang Weijun]

> On Nov 2, 2016, at 9:18 PM, Xuelei Fan <xuelei.fan at oracle.com> wrote:
> 
> On 11/2/2016 8:47 PM, Wang Weijun wrote:
>> 
>>> On Nov 2, 2016, at 5:34 PM, Xuelei Fan <xuelei.fan at oracle.com> wrote:
>>> 
>>> 1.  More specific
>>> 
>>>  "A SecureRandom service provider can advertise that it is
>>>   thread-safe by setting the service provider attribute
>>>   "ThreadSafe" to "true" when registering the provider."
>>> 
>>> A service provider may contains many services implementations.  May need to be more specific to set "ThreadSafe" for SecureRandom only, rather the full provider is thread safe.  For example:
>>> 
>>>   map.put("SecureRandom.SHA1PRNG ThreadSafe", "true");
>>> 
>>> Otherwise, a service provider need to make sure all services are thread safe, or all services implementation are not thread safe.
>> 
>> How about changing "A SecureRandom service provider" to "A SecureRandom implementation"?
>> 
> I don't think it is sufficient.  See bellow.
> 
>>> 
>>> 2. "true" is the only true property value.
>>>   "If this attribute is not set or is "false", this class will
>>>    instead ..."
>>> 
>>> If the attribute is set to "yes" or "hello, world!", I think it is the same as set to "false" per your current implementation.
>>> 
>>>   "Otherwise, this class will ... "
>> 
>> OK.
>> 
>>> 
>>> May need to update the implementation accordingly if you accept the comments.
>> 
>> Why update the implementation?
>> 
> If I'm reading correct, you are setting the "ThreadSafe" for provider, but not for SecureRandom implementation.

Sorry but I don't understand what you mean.

On read side, the threadSafe field is for each "SecureRandom." + algorithm, so it is for an implementation.

On write side, every call looks like the line you write below.

>  I may use the property similar to:
> 
>   map.put("SecureRandom.SHA1PRNG ThreadSafe", "true");
> 
> That's why I don't think above update is not sufficient.

Can you point out on exactly which line in webrev I'm doing wrong?

Thanks
Max


> 
> Xuelei
> 
>> Thanks
>> Max
>> 
>>> 
>>> Xuelei
>>> 
>>> 
>>> On 11/2/2016 3:27 PM, Wang Weijun wrote:
>>>> Ping again.
>>>> 
>>>> There is an updated version at http://cr.openjdk.java.net/~weijun/7004967/webrev.01/ with doc-only changes.
>>>> 
>>>> Thanks
>>>> Max
>>>> 
>>>>> On Aug 25, 2016, at 10:00 AM, Weijun Wang <weijun.wang at oracle.com> wrote:
>>>>> 
>>>>> Please review the enhancement at
>>>>> 
>>>>> http://cr.openjdk.java.net/~weijun/7004967/webrev.00/
>>>>> 
>>>>> Basically, we want SecureRandom to be more efficient by removing all synchronized keywords from its public methods and let an implementation to take care of thread-safety (We already did some in JDK-8098581). On the other hand, we need to make sure that existing implementations that have not synchronized correctly to behave just as good as before.
>>>>> 
>>>>> Therefore a new Service Attribute "ThreadSafe" is introduced. If you think your implementation is already thread-safe, set it to "true" and SecureRandom will be happy. Otherwise, don't set it and SecureRandom will continuously call your SecureRandomSpi engine methods in synchronized blocks.
>>>>> 
>>>>> Thanks
>>>>> Max