-1

Just want to know how the below codes that does the same functionality differs

Code 1:

    class ReadWriteCounter {
    ReadWriteLock lock = new ReentrantReadWriteLock();

    private Integer count = 0;

    public Integer incrementAndGetCount() {
        lock.writeLock().lock();
        try {
            count = count + 1;
            return count;
        } finally {
            lock.writeLock().unlock();
        }
    }

    public Integer getCount() {
        lock.readLock().lock();
        try {
            return count;
        } finally {
            lock.readLock().unlock();
        }
    }
}

Code 2:

class ReadWriteCounter {

private Integer count = 0;

public getCount()
{
   synchronized(count){
   return count;
   }
}

public void setCount(Integer i)
{
     synchronized(count){
         count = i;
       }
   }
}

The purpose is to ensure that when count is modified no other threads access it for reading and while reading no other threads should should access it for writing. Which is an optimum solution and why? Also, I will be using this in a class where there are field variables which needs to edited. Please offer your suggestions.

Kiran Cyrus Ken
  • 379
  • 1
  • 3
  • 17
  • 1
    Is not an answer to the question, though using [AtomicInteger#incrementAndGet](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/atomic/AtomicInteger.html#incrementAndGet--) method would be an alternative less verbose for this use case –  Apr 02 '19 at 06:55

2 Answers2

1

ReentrantReadWriteLock is the best way to implement your thoughts. synchronized would only allow one thread if two or more threads attempt to read count. But everyone could get the value of count when they all attempt to read it.

Sean Lee
  • 46
  • 3
  • 1
    AtomicInteger is the best way in this particular example. For most use-cases, there are existing datastructures in the concurrency utils package, so that you should always think twice if you really need to touch low-level stuff like locks directly. – Thilo Apr 02 '19 at 07:08
  • @Thilo Are you seriously? Note this part of the problem: "The purpose is to ensure that when count is modified no other threads access it for reading"; You will get the old value when another one is updating the 'count' if you use AtomicInteger here. CAS in AtomicInteger could not guarantee it. – Sean Lee Apr 02 '19 at 08:15
  • 1
    @SeanLee In this use case it should be fine according to AtomicInteger [javadoc](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/atomic/AtomicInteger.html) "An AtomicInteger is used in applications such as atomically incremented counters, and cannot be used as a replacement for an Integer" and [more details on CAS with atomics](https://dzone.com/articles/how-cas-compare-and-swap-java) also suggests the CAS using atomic variables usually is good enough –  Apr 02 '19 at 09:32
1

Both your solutions work however there is a bug in the way you are implementing locking.

First the difference in the two approaches: The ReentrantReadWriteLock is mainly used in situations wherein you have many more reads than writes typically in ratios of 10 reads : 1 write. This allows the reads to happen concurrently without blocking each other however when a write starts all reads will be blocked. So performance is the primary reason.

Bug in your approach : The object on which you are locking should be final. In setCount() you are effectively swapping the object out and that can cause a dirty read at that time.

Also, never expose the object that you are locking on. The object you are locking should be private and final. The reason is if you happen to expose the object the caller may happen to use the returned object itself for locking, in which case you will run into contention issues with components outside this class itself.

Kiran K
  • 703
  • 4
  • 11