1

My main question is "Do I really need recheck the value in the following code"?
The following code describes how to implement a cache by using ReadWriteLock. 

public class ReadWriteLockCache {
private Map<String, Object> cache = new HashMap<String, Object>();
private ReadWriteLock rwl = new ReentrantReadWriteLock();

/**
 * @param args
 */
public static void main(String[] args) {
    // TODO Auto-generated method stub
}

public Object getData(String key) {
    rwl.readLock().lock();

    Object value = null;

    try {
        value = cache.get(key);

        if (value == null) {
            rwl.readLock().unlock(); //line1
            rwl.writeLock().lock(); //line2

            try {
                if (value == null) //line3
                 {
                    value = "aaaa"; // may get data from db
                                    //line5 put the data into cache.

                    cache.put(key, value);
                }
            } finally {
                rwl.writeLock().unlock();
            }

            rwl.readLock().lock();
        }
    } finally {
        rwl.readLock().unlock();
    }

    return value;
}

}

Whether Should I still need to recheck the value at the 'line3' ?

As I know, the value object is must be null when excuted to line3,
because it's a local variable (when it's null)and it's cann't be A state variable of our main obj:ReadWriteLockCache.
What we should really do is recall the get method ,and check the key's value is whether putted by some other thread or not.

The code should like this:

                    value = cache.get(key);
                if (value == null) //line3
                 {
                    value = "aaaa"; 
                    cache.put(key, value);
                }

Anyone can help?Am I right? I'm confused.

scugxl
  • 317
  • 4
  • 15
  • Where did you get this example for ReadWriteLockCache from? – peter.petrov Oct 15 '15 at 15:27
  • You could probably achieve the same with a ConcurrentHashMap and only a few lines of code. – assylias Oct 15 '15 at 15:37
  • @peter.petrov I got the code from a website ,I can stil find the code in many search engine results.That's why I'm so confused, because I can't get the correct implemention. – scugxl Oct 15 '15 at 15:43
  • @assylias I guess you mean the code solution is to use the putIfAbsent in ConcurrentHashMap ? I use RWL to do this is beacause the performacne. In my situation, read threads or counts is much more than writes. – scugxl Oct 15 '15 at 15:46
  • @scugxl CHM does not lock on `get` operations so I would not be so sure that your approach performs better to be honest... You should run performance tests before reaching any conclusions. – assylias Oct 15 '15 at 15:49
  • And the method getData body could be replaced with : return concurrentHashMap.computeIfAbsent(key, k -> callDb(k)); which will be easier to maintain and will contain less bugs (hopefully) – Thierry Oct 15 '15 at 15:56
  • @assylias u may be right, but that's not my question.for the code , any idea? – scugxl Oct 16 '15 at 10:51

1 Answers1

0

Yes you should as read lock can be read by multiple threads at the same time there might be one thread executing line 3 and other thread might be waiting in line 2

But as other suggested use concurrent hashmap

user3214003
  • 1
  • 1