RFR 7004967: SecureRandom should be more explicit about threading

[Xuelei Fan]

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

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