Code review request: 7063647, jsse/runtime, To use synchronized map in key manager

[Xuelei Fan]

Since the all writes done during the constructor, we don't need to use
synchronized map for credentialsMap. The HashMap.iterator() is
thread-safe for read-only HashMap.

new webrev: http://cr.openjdk.java.net/~xuelei/7063647/webrev.01/

Xuelei

On 8/9/2011 8:58 AM, Xuelei Fan wrote:
> On 8/9/2011 8:03 AM, Brad Wetmore wrote:
>> Why do you need the "syncronchized (credentialsMap)" at line 344? Aren't
>> all writes done during the constructor init?
>>
> The credentialsMap.entrySet() returns the reference of the
> backed/internal set, the iteration of the set is not thread safe. The
> Collections.synchronizedMap specification requires:
> ------------------
> It is imperative that the user manually synchronize on the returned map
> when iterating over any of its collection views:
> 
>   Map m = Collections.synchronizedMap(new HashMap());
>       ...
>   Set s = m.keySet();  // Needn't be in synchronized block
>       ...
>   synchronized (m) {  // Synchronizing on m, not s!
>       Iterator i = s.iterator(); // Must be in synchronized block
>       while (i.hasNext())
>           foo(i.next());
>   }
> 
> 
> Failure to follow this advice may result in non-deterministic behavior.
> ------------------
> 
> Thanks for the code review.
> 
> Xuelei
> 
>> Otherwise, looks ok.
>>
>> Brad
>>
>>
>> On 8/7/2011 8:43 PM, Xuelei Fan wrote:
>>> webrev: http://cr.openjdk.java.net/~xuelei/7063647/webrev.00/
>>>
>>> SunX509KeyManagerImpl should be multiple thread safe, need to
>>> synchronize cached map:
>>>      private Map<String,X509Credentials>  credentialsMap;
>>>      private Map<String,String[]>  serverAliasCache;
>>>
>>> Thanks,
>>> Xuelei
>