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

[Xuelei Fan]

On 8/15/2011 11:28 AM, Weijun Wang wrote:
> 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?
> 
Yes.

I don't want to synchronize the get/put block because of performance and
potential deadlock issues. Once the alias is out into the cache, line
262~267 will not be executed any more.

> If so, I'm fine with the code change.
> 
Thanks for the code review.

Xuelei (Andrew)

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