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 >
