Code review request: 7063647, jsse/runtime, To use synchronized map in key manager
Xuelei Fan
xuelei.fan at oracle.com
Fri Aug 12 01:24:03 UTC 2011
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
>
More information about the security-dev
mailing list