On Sun, Jul 13, 2014 at 2:48 PM, Andreas Veithen <[email protected]> wrote:
> That doesn't look thread safe: > * Calling containsKey is not the same as a null check. > true, but no one modifies the key/value pair once it is written so checking key is essentially same as checking value. > * "double checked locking" is an anti-pattern because it is unsafe. > * In addition to that, you added an unsynchronized call to the get method. > I am aware of the java memory model and partial initialization problem with DCL. However I do not necessarily believe it is an issue for this specific scenario. First we check for the key in map#containsKey() which returns an boolean which is a 32 bit primitive. 32 bit primitives are atomic and do not cause partial writes within JVM. Secondly concurrent hash map and related methods are synchronized which guarantees a read/write memory barrier within the call block. Therefore I think it should be the case that the map has been atomically synchronized (or atomically not) for all its state between memory/local cache once the check is made (even if we assume that this is not the case final map#get() should execute the respective memory barriers. This will synchronize members between threads so that existence of any partial data of the map is eliminated ). > Andreas > > On Sun, Jul 13, 2014 at 7:15 PM, <[email protected]> wrote: > > Author: uswick > > Date: Sun Jul 13 18:14:59 2014 > > New Revision: 1610262 > > > > URL: http://svn.apache.org/r1610262 > > Log: > > avoid sync overhead each time null is checked - use double checked > locking > > > > Modified: > > > synapse/trunk/java/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/connections/TargetConnections.java > > > > Modified: > synapse/trunk/java/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/connections/TargetConnections.java > > URL: > http://svn.apache.org/viewvc/synapse/trunk/java/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/connections/TargetConnections.java?rev=1610262&r1=1610261&r2=1610262&view=diff > > > ============================================================================== > > --- > synapse/trunk/java/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/connections/TargetConnections.java > (original) > > +++ > synapse/trunk/java/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/connections/TargetConnections.java > Sun Jul 13 18:14:59 2014 > > @@ -205,16 +205,16 @@ public class TargetConnections { > > > > private HostConnections getConnectionPool(String host, int port) { > > String key = host + ":" + port; > > - HostConnections pool; > > - synchronized (poolMap) { > > - // see weather a pool already exists for this host:port > > - pool = poolMap.get(key); > > - if (pool == null) { > > - pool = new HostConnections(host, port, maxConnections); > > - poolMap.put(key, pool); > > + if (!poolMap.containsKey(key)) { > > + synchronized (poolMap) { > > + // see weather a pool already exists for this host:port > > + if (!poolMap.containsKey(key)) { > > + HostConnections pool = new HostConnections(host, > port, maxConnections); > > + poolMap.put(key, pool); > > + } > > } > > } > > - return pool; > > + return poolMap.get(key); > > } > > > > } > > > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > > -- http://www.udayangawiki.blogspot.com
