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

Jeremy Manson jeremy.manson at gmail.com
Sun Jul 1 14:29:32 EDT 2012


The counter field should be final, because another thread could read
the enclosing object before it is finished being constructed (unless
youve taken other precautions to prevent that.

Jeremy

On Fri, Jun 29, 2012 at 4:24 AM, Pavel Rappo <pavel.rappo at gmail.com> wrote:
> 1. I can't see any obvious flaws in it.
> 2. http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/package-summary.html
> (see "Memory Consistency Properties")
>
> On 6/29/12, Li Li <fancyerii at gmail.com> wrote:
>> do you mean my counter is safe?
>> putIfAbsent has a happen-before semantic? any document about this? Or
>> only current implementation guarantee this?
>>
>> On Fri, Jun 29, 2012 at 6:54 PM, Pavel Rappo <pavel.rappo at gmail.com> wrote:
>>> 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.
>>
>
>
> --
> Sincerely yours, Pavel Rappo.
> _______________________________________________
> Concurrency-interest mailing list
> Concurrency-interest at cs.oswego.edu
> http://cs.oswego.edu/mailman/listinfo/concurrency-interest


More information about the Concurrency-interest mailing list