[concurrency-interest] [Javamemorymodel-discussion] is my counter thread safe?

Pavel Rappo pavel.rappo at gmail.com
Fri Jun 29 06:54:14 EDT 2012


Hi,

This 'counter.get(key)' cannot simply return "not well constructed"
object. The reason is that you use 'putIfAbsent' to put it in the map.
Either 'get' will return 'null' or it will return perfectly valid
AtomicLong object. There's a happen-before edge between these two
actions.

On 6/29/12, Li Li <fancyerii at gmail.com> wrote:
> hi all
>    I have read
> http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
> and know a little about Java memory model.
>    I want to implement a thread safe and efficient Map<String,int>
> counter. I found a related question in stackoverflow
> http://stackoverflow.com/questions/8477352/global-in-memory-counter-that-is-thread-safe-and-flushes-to-mysql-every-x-increm
>    my implementation is :
>
>         private ConcurrentHashMap<String, AtomicLong> counter=new
> ConcurrentHashMap<String, AtomicLong>();
> 	
> 	public void addCount(String key, long count){
> 		if(count<=0) return;
> 		AtomicLong current = counter.get(key);
>                 if(current == null) {
>         	    current=counter.putIfAbsent(key, new AtomicLong());
>         	    if(current == null) current=counter.get(key);
>                 }
>
>                 assert current!=null;
>                 current.addAndGet(count);
>
> 	}
>    but after I reading
> http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
> . it seems there exists a state that may fail.
>    if thread1 call addCount("key",2); and thread2 call
> addCount("key",1); at the same time.
>    thread1 executes AtomicLong current = counter.get(key);  and gets null.
>    then thread 1 execute
>           if(current == null) {
>         	    current=counter.putIfAbsent(key, new AtomicLong());
>    as compilers/cpus may disorder, new AtomicLong() will not be null
> but may be well constructed.
>    Then thread 2 call AtomicLong current = counter.get(key); it's not
> null but not well constructed. then it call current.addAndGet().
>     Will thread 2 crash if current is not well constructed?
>
>     I also find similar implementation in a popular lib -- guava
> https://code.google.com/p/guava-libraries/source/browse/guava/src/com/google/common/util/concurrent/AtomicLongMap.java?name=v11.0-rc1
>     it may also fail like my codes.
>     e.g. thread 1 call atomic = map.putIfAbsent(key, new
> AtomicLong(delta));
>            thread 2 get a not null atomic and call it's get();
>
> public long addAndGet(K key, long delta) {
>     outer: for (;;) {
>       AtomicLong atomic = map.get(key);
>       if (atomic == null) {
>         atomic = map.putIfAbsent(key, new AtomicLong(delta));
>         if (atomic == null) {
>           return delta;
>         }
>         // atomic is now non-null; fall through
>       }
>
>       for (;;) {
>         long oldValue = atomic.get();
>         if (oldValue == 0L) {
>           // don't compareAndSet a zero
>           if (map.replace(key, atomic, new AtomicLong(delta))) {
>             return delta;
>           }
>           // atomic replaced
>           continue outer;
>         }
>
>         long newValue = oldValue + delta;
>         if (atomic.compareAndSet(oldValue, newValue)) {
>           return newValue;
>         }
>         // value changed
>       }
>     }
>   }
> _______________________________________________
> Javamemorymodel-discussion mailing list
> Javamemorymodel-discussion at cs.umd.edu
> https://mailman.cs.umd.edu/mailman/listinfo/javamemorymodel-discussion
>


-- 
Sincerely yours, Pavel Rappo.


More information about the Concurrency-interest mailing list