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

[Weijun Wang]

So you mean you don't really care if the serverAliasCache.put(*,*) 
method would be executed twice for the same keyType, because the value 
will always be the same and there will be no harm made. Right?

If so, I'm fine with the code change.

[ I add CC back, otherwise no one can prove you get a yes for the code 
review ]

-Max


On 08/15/2011 11:09 AM, Xuelei Fan wrote:
> On 8/15/2011 11:02 AM, Xuelei Fan wrote:
>> On 8/15/2011 10:35 AM, Weijun Wang wrote:
>>> I'm not sure what action on serverAliasCache you want to protect.
>>>
>>> For example,
>>>
>>>   260             aliases = serverAliasCache.get(keyType);
>>>   261             if (aliases == null) {
>>>   262                 aliases = getServerAliases(keyType, issuers);
>>>   263                 // Cache the result (positive and negative lookups)
>>>   264                 if (aliases == null) {
>>>   265                     aliases = STRING0;
>>>   266                 }
>>>   267                 serverAliasCache.put(keyType, aliases);
>>>   268             }
>>>
>>> Here it's still possible that two threads run at the same time and both
>>> going into lines 262. Is this what you want to see?
>>>
>> Not exactly, I want to ensure that when one thread works on line 260,
>> another thread does not work on 267.
>>
> The logic of line 262 is a little strange that it will always return the
> same value. So I don't worried about it.
>
> Andrew
>
>> Thanks,
>> Andrew
>>
>>> Thanks
>>> Max
>>>
>>>
>>> On 08/14/2011 08:49 AM, Xuelei Fan wrote:
>>>> Max,
>>>>
>>>> Are you available to review this simple fix?
>>>>
>>>> Thanks,
>>>> Andrew
>>>>
>>>> -------- Original Message --------
>>>> Subject: Re: Code review request: 7063647, jsse/runtime, To use
>>>> synchronized map in key manager
>>>> Date: Fri, 12 Aug 2011 09:24:03 +0800
>>>> From: Xuelei Fan<xuelei.fan at oracle.com>
>>>> To: Brad Wetmore<bradford.wetmore at oracle.com>
>>>> CC: OpenJDK<security-dev at openjdk.java.net>
>>>>
>>>> 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
>>>>>
>>>>
>>
>