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<[email protected]>
To: Brad Wetmore<[email protected]>
CC: OpenJDK<[email protected]>
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
