Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am trying to implement lock by which I don't want to have reads from happening whenever I am doing a write.

Below is my ClientData class in which I am using CountDownLatch -

public class ClientData {

    private static final AtomicReference<Map<String, Map<Integer, String>>> primaryMapping = new AtomicReference<>();
    private static final AtomicReference<Map<String, Map<Integer, String>>> secondaryMapping = new AtomicReference<>();
    private static final AtomicReference<Map<String, Map<Integer, String>>> tertiaryMapping = new AtomicReference<>();

    // should this be initialized as 1?
    private static final CountDownLatch hasBeenInitialized = new CountDownLatch(1) 

    public static Map<String, Map<Integer, String>> getPrimaryMapping() {
        try {
            hasBeenInitialized.await();
        } catch (InterruptedException e) {
            throw new IllegalStateException(e);
        }

        return primaryMapping.get();
    }

    public static void setPrimaryMapping(Map<String, Map<Integer, String>> map) {
        primaryMapping.set(map);
        hasBeenInitialized.countDown();
    }

    public static Map<String, Map<Integer, String>> getSecondaryMapping() {
        try {
            hasBeenInitialized.await();
        } catch (InterruptedException e) {
            throw new IllegalStateException(e);
        }

        return secondaryMapping.get();
    }       

    public static void setSecondaryMapping(Map<String, Map<Integer, String>> map) {
        secondaryMapping.set(map);
        hasBeenInitialized.countDown();
    }

    public static Map<String, Map<Integer, String>> getTertiaryMapping() {
        try {
            hasBeenInitialized.await();
        } catch (InterruptedException e) {
            throw new IllegalStateException(e);
        }

        return tertiaryMapping.get();
    }       

    public static void setTertiaryMapping(Map<String, Map<Integer, String>> map) {
        tertiaryMapping.set(map);
        hasBeenInitialized.countDown();
    }       
}

Problem statement:

I need to wait on the get calls on three AtomicReferences I have in the above code. Once all the writes has been done on the three AtomicReferences I have with the set call, then I would allow making the call to three getters which I have.

So I decided to use CountDownLatch which I have initialized as 1? Do I need to initialize it to 3? And every time before I do the first set on a new update, should I need to resetup the countdown latch back to 3? Because I will be setting those three AtomicReferences in separate three statements.

Is there something wrong in my above code?

Some other threads has to read the data from these AtomicReferences once they have been set.

Below is my background thread code which will get the data from the URL, parse it and store it in a ClientDataclass variable.

public class TempScheduler {

    private final ScheduledExecutorService scheduler = Executors.newScheduledThreadPool(1);

        public void startScheduler() {
            final ScheduledFuture<?> taskHandle = scheduler.scheduleAtFixedRate(new Runnable() {
                public void run() {
                try {
                    callServers();
                } catch (Exception ex) {
                    ex.printStackTrace();
                }
                }
            }, 0, 10, TimeUnit.MINUTES);
        }
    }

    // call the servers and get the data and then parse 
    // the response.
    private void callServers() {
        String url = "url";
        RestTemplate restTemplate = new RestTemplate();
        String response = restTemplate.getForObject(url, String.class);
        parseResponse(response);

    }

    // parse the response and store it in a variable
    private void parseResponse(String response) {
        //...       
        ConcurrentHashMap<String, Map<Integer, String>> primaryTables = null;
        ConcurrentHashMap<String, Map<Integer, String>> secondaryTables = null;
        ConcurrentHashMap<String, Map<Integer, String>> tertiaryTables = null;

        //...

        // store the data in ClientData class variables which can be
        // used by other threads
        ClientData.setPrimaryMapping(primaryTables);
        ClientData.setSecondaryMapping(secondaryTables);
        ClientData.setTertiaryMapping(tertiaryTables);
    }
}

Opting for a code review here. Is there any better way of doing as the way I am doing currently? And is there any way I can use one latch instead of three separate latches here?

share|improve this question
    
Is this a Bean or something (i.e. why do you need three setters... can't you setMappings(primary, secondary, tertiary)) ? –  rolfl Apr 28 at 22:39
    
@rolfl: I have updated the question. Can you take a look whether that's what you were suggesting? And with the above code, I can use countdownlatch as 1, right? I still doubt whatever I did is right? –  Webby Apr 28 at 22:59
    
From your code it seems that the background thread will be overwriting the maps every so often. Is that correct? –  David Harkness Apr 29 at 1:13
    
@David: Not everytime, only whenever there is any change. For the first time, when the code is run, then it will add the values in the map. And then after that, whenever there is any change then only it will update those maps and that change will happen only once in three months max. So that means if the code is running for three months, and if we decide to change something on the service url which my background thread is parsing, and if there is any change, then only it will update those three maps. –  Webby Apr 29 at 1:23
    
@Webby One change every three months is more than none which means this code must accept a new set of maps at any time. –  David Harkness Apr 29 at 1:27

2 Answers 2

up vote 6 down vote accepted

The fact that you must be able to swap in a new set of maps at any time--even if only once every three months--requires a design change. Here are the requirements as I understand them:

  • Reads block until all three maps have been set the first time.
  • Reads receive a consistent set of maps. In other words, you cannot return the old secondary map after returning the new primary map.

Taken together, you really must combine the maps into a new data structure which is returned to clients and replaced in whole by the background thread. Storing it in its own atomic reference allows this new class to avoid concurrency and locks altogether.

  • Keep the latch to block reads before the first set of maps have been loaded.
  • Combine the three maps in a simple data holder.
  • Store the data holder in an atomic reference (if necessary) instead of each map individually.
  • Set all three maps in one call by storing a new data holder instance. Since all three are stored in one call, the latch can use an initial count of 1.

Update

Now let's throw some code behind the above changes.

public class ClientData {

    public static class Mappings {
        public final Map<String, Map<Integer, String>> primary;
        public final Map<String, Map<Integer, String>> secondary;
        public final Map<String, Map<Integer, String>> tertiary;

        public Mappings(
            Map<String, Map<Integer, String>> primary,
            Map<String, Map<Integer, String>> secondary,
            Map<String, Map<Integer, String>> tertiary
        ) {
            this.primary = primary;
            this.secondary = secondary;
            this.tertiary = tertiary;
        }
    }

    private static final AtomicReference<Mappings> mappings = new AtomicReference<>();
    private static final CountDownLatch hasBeenInitialized = new CountDownLatch(1);

    public static Mappings getMappings() {
        try {
            hasBeenInitialized.await();
            return mappings.get();
        } catch (InterruptedException e) {
            Thread.currentThread().interrupt(); // @rolfl covered this
            throw new IllegalStateException(e);
        }
    }

    public static void setMappings(
        Map<String, Map<Integer, String>> primary,
        Map<String, Map<Integer, String>> secondary,
        Map<String, Map<Integer, String>> tertiary
    ) {
        setMappings(new Mappings(primary, secondary, tertiary));
    }

    public static void setMappings(Mappings newMappings) {
        mappings.set(newMappings);
        hasBeenInitialized.countDown();
    }
}

There are some other improvements I would suggest but did not implement.

  • Avoid using static methods when possible to ease testing and alternate implementations.
  • The atomic reference may not actually be required since both get and set operations synchronize on hasBeenInitialized.
  • If clients of ClientData can handle receiving empty mappings, you can drop the latch and initialize the class with three empty maps. This would reduce all synchronization to the atomic reference.
share|improve this answer
    
For your second point, Yes, I can still return old map while I am updating the maps second time. But as soon the update is done, I should return new maps value. Sorry for the confusion if I was not clear earlier. –  Webby Apr 29 at 1:46
    
But is it okay to return the new first map and the old second/third map to the same caller? If they are related, you should return either all old maps or new maps--never a random mix of the two to the same caller. –  David Harkness Apr 29 at 1:49
    
No. In that case, it should either return all the old maps or all the new maps , not intermix of it. Right now I am using @rolfl suggestion which uses the ReentrantLock. Do you think this use case will be solve by that or there will be any problem? –  Webby Apr 29 at 1:52
    
@Webby My understanding of rolfl's code is that it doesn't allow setting a new trio of maps. Did I miss something? –  David Harkness Apr 29 at 2:21
    
He has the set calls right which I can use to set them up? –  Webby Apr 29 at 2:39

AtomicReferences are a great construct when you have just one item that needs to be kept in a sane state ina multithreaded environment. Your code is trying to juggle three, and ensure they all have a sane state at the right times, and, for that, you need something bigger than the AtomicReference.

You have elected to use the CountDownLatch. This, frankly, in this situation, is not the right choice. The right choice is to use basic synchronization, or to use a ReentrantLock.

I have taken the liberty of rewriting your class using a Lock, with a dependent Condition, and no other java.util.* components.

This code waits for all three maps to be set, and, when they are, it signals the Condition, and the getters are 'released'.

import java.util.Map;
import java.util.concurrent.locks.Condition;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;

@SuppressWarnings("javadoc")
public class ClientData {

    private static final class MapContainer {
        private Map<String, Map<Integer, String>> value = null;

        public Map<String, Map<Integer, String>> getValue() {
            return value;
        }

        public void setValue(Map<String, Map<Integer, String>> value) {
            this.value = value;
        }

    }

    private static final MapContainer primaryMapping = new MapContainer();
    private static final MapContainer secondaryMapping = new MapContainer();
    private static final MapContainer tertiaryMapping = new MapContainer();
    private static final MapContainer[] containers = {primaryMapping, secondaryMapping, tertiaryMapping};
    private static boolean allset = false;
    private static final Lock lock = new ReentrantLock();
    private static final Condition allsetnow = lock.newCondition();

    private static final Map<String, Map<Integer, String>> getMapping(MapContainer container) {
        lock.lock();
        try {
            while (!allset) {
                allsetnow.await();
            }
            return container.getValue();
        } catch (InterruptedException e) {
            Thread.currentThread().interrupt(); // reset interruptedd state.
            throw new IllegalStateException(e);
        } finally {
            lock.unlock();
        }

    }

    private static final void setMapping(MapContainer container, Map<String, Map<Integer, String>> value) {
        if (value == null) {
            throw new IllegalArgumentException("Null Map cannot be used");
        }
        lock.lock();
        try {
            if (allset) {
                throw new IllegalStateException("All the maps are already set");
            }
            if (container.getValue() != null) {
                throw new IllegalArgumentException("Cannot change the value in a mapping");
            }
            container.setValue(value);
            for (MapContainer cont : containers) {
                if (cont.getValue() == null) {
                    // not all values are set....
                    return;
                }
            }
            allset = true;
            allsetnow.signalAll();
        } finally {
            lock.unlock();
        }
    }

    public static Map<String, Map<Integer, String>> getPrimaryMapping() {
        return getMapping(primaryMapping);
    }

    public static void setPrimaryMapping(Map<String, Map<Integer, String>> map) {
        setMapping(primaryMapping, map);
    }

    public static Map<String, Map<Integer, String>> getSecondaryMapping() {
        return getMapping(secondaryMapping);
    }

    public static void setSecondaryMapping(Map<String, Map<Integer, String>> map) {
        setMapping(secondaryMapping, map);
    }

    public static Map<String, Map<Integer, String>> getTertiaryMapping() {
        return getMapping(tertiaryMapping);
    }       

    public static void setTertiaryMapping(Map<String, Map<Integer, String>> map) {
        setMapping(tertiaryMapping, map);
    }       
}

It would be better if, instead of three separate set* methods, you had just one method that set all three maps.... if you did, the code would look simpler, like:

    public static void setAllMappings(Map<String, Map<Integer, String>> primary,
            Map<String, Map<Integer, String>> secondary,
            Map<String, Map<Integer, String>> tertiary) {
        lock.lock();
        try{
            if (allset) {
                throw new IllegalStateException("Maps already set");
            }
            primaryMapping.setValue(primary);
            secondaryMapping.setValue(secondary);
            tertiaryMapping.setValue(tertiary);
            allset = true;
            allsetnow.signalAll();
        } finally {
            lock.unlock();
        }
    }

Other problems in your code are:

  • your use of the CountDownLatch should have started at 3
  • it is a problem if the same setter is called twice (will reach 0 before all maps are set)
  • When handling InterruptedException, you should leave the thread in an appropriate state. In your case, by catching it, and wrapping it in a RuntimeException, you should also reset the interrupted state of the thread.

Edit:

If you were to adjust the way you set the values in the AtomicReference, then the Concerns I have with the CountDownLatch would be mitigated. Consider the following:

public static void setPrimaryMapping(Map<String, Map<Integer, String>> map) {
    if (map != null && primaryMapping.compareAndSet(null, map)) {
        hasBeenInitialized.countDown();
    } else {
        throw new IllegalSateException("Map has already been set... cannot double-set it");
    }
}

The above will mean that each Map can be set only once, and the countdownlatch will work well enough (if initialized with 3).

Note that with the CountDown latch, and 3 AtomicReferences, that you have 4 locks that all need to work together, when really, the only one that is important for the actual acess restrictions is the CountDownLatch. That is the reason I don't like the solution with the atomics, and the latch, because the same can be accomplished with just a single lock, which applies the access and concurrency restrictions in a more consistent/logical way.

Edit 2

Simple traditional synchronization mechanism.

import java.util.Map;

@SuppressWarnings("javadoc")
public class ClientData {

    private static Map<String, Map<Integer, String>> primaryMap = null;
    private static Map<String, Map<Integer, String>> secondaryMap = null;
    private static Map<String, Map<Integer, String>> tertiaryMap = null;
    private static boolean allset = false;
    private static final Object synclock = new Object();

    private static final void waitTillSet() {
        try {
            while (!allset) {
                synclock.wait();
            }
        } catch (InterruptedException e) {
            Thread.currentThread().interrupt();
            throw new IllegalStateException(e);
        }
    }

    public static void setAllMappings(Map<String, Map<Integer, String>> primary, 
            Map<String, Map<Integer, String>> secondary, 
            Map<String, Map<Integer, String>> tertiary) {
        synchronized (synclock) {
            if (allset) {
                return; // throw exception otherwise.
            }
            primaryMap = primary;
            secondaryMap = secondary;
            tertiaryMap = tertiary;
            allset = true;
            synclock.notifyAll();
        }
    }

    public static Map<String, Map<Integer, String>> getPrimaryMapping() {
        synchronized (synclock) {
            waitTillSet();
            return primaryMap;
        }
    }

    public static Map<String, Map<Integer, String>> getSecondaryMapping() {
        synchronized (synclock) {
            waitTillSet();
            return secondaryMap;
        }
    }

    public static Map<String, Map<Integer, String>> getTertiaryMapping() {
        synchronized (synclock) {
            waitTillSet();
            return tertiaryMap;
        }
    }       

}
share|improve this answer
    
@Webby - the first block of code is compatible with what you originally posted. It has three separate set*Mapping(Map...) calls. The second block of code is an alternate mechanism that sets all three at once, and does not need to check if the others have been set.... You should either use the code as it is in the first block, or, replace the three set*Mapping methods in the first block with the one setAllMappings(...) in the second block –  rolfl Apr 28 at 23:37
    
Yup.. I figure that one out after taking a close look at the code. In general what is the benefit of ReentrantLock as compared to I was using CountDownLatch earlier? And here ReentrantLock means, that as soon as the write is done on those three maps, then only you will be able to read the data from those three maps, right? And again, if the writes are happening to those three tables, then the call will be blocked and whenever the writes are done, then the thread will be able to read the data again using the getters? –  Webby Apr 28 at 23:53
    
@Webby - Hmmm ... those are huge things to answer.... The big problem I have with the countdownLatch is the unsafe setting... you can set the same map 3 times, and trigger the getters... Let me add an update to my answer. –  rolfl Apr 29 at 0:00
    
Thanks. In this case also with ReentrantLock, once the first write is done on all three tables, then there won't be any locking happening on the get calls, Right? Only for the first write to those three tables, there will be a locking on get calls to those three tables otherwise there won't be any? –  Webby Apr 29 at 0:20
1  
There will always be locking on the get*(...) calls, (whether with the CountdownLatch or the ReentrantLock). There will only be blocking if the gets are called before all three sets are called. –  rolfl Apr 29 at 0:22

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.