4
\$\begingroup\$

This is a follow-up to: Lock for preventing concurrent access in client data

I am trying to implement lock by which I want to avoid reads from happening whenever I am doing a write on my three maps.

My requirements are:

  1. Reads block until all three maps have been set for the first time.
  2. Now second time, If I am updating the maps, I can still return all the three old maps value(before the updates are done on all three maps) or it should block and return me all the new three maps value whenever the updates are done on all the three maps.

As I have three Maps - primaryMapping, secondaryMapping and tertiaryMapping, it should return either all the new values of three updated maps or it should return all the old values of the map. Basically, while updating I don't want to return primaryMapping having old values, secondaryMapping having having new values, and tertiaryMapping with new values.

It should be consistent, either it should return old values or it should return new values after updating the maps. In my case, updating of maps will happen once in three months or four months.

Here is my ClientData class in which I am using ReentrantLock in which the whole logic is there:

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();
        }

    }

    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{

            // how  to avoid this?
            if (allset) {
                throw new IllegalStateException("All the maps are already set");
            }

            primaryMapping.setValue(primary);
            secondaryMapping.setValue(secondary);
            tertiaryMapping.setValue(tertiary);
            allset = true;
            allsetnow.signalAll();
        } finally {
            lock.unlock();
        }
    }        


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

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

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

Here is my background thread code which will get the data from my service URL and keep on running every 10 minutes once my application has started up. It will then parse the data coming from the URL and store it in a ClientData class variable in those three maps.

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 if anything has changed  
        // which can be used by other threads
        if(changed) {
            ClientData.setAllMappings(primaryTables, secondaryTables, tertiaryTables);
        }
    }
}

I will be using getPrimaryMapping, getSecondaryMapping and getTertiaryMapping in the ClientData class from the main thread in my main appliction so I want to return either all the new set of values from these three maps or if update is happening, then either block it and return all the new set of values for those three maps after the updates are done.

In the ClientData class, I guess I won't be able to update the maps once it has been set for the first time as this line will cause a problem and throw an exception. Also, how can I implement my second point as shown above?

// how to avoid this?
if (allset) {
    throw new IllegalStateException("All the maps are already set");
}

How can I successfully implement all my above two points? I guess there is something very minor thing which I am missing here. I want to use ReentrantLock here but any other suggestions are also welcome.

Initially, I was thinking to remove this if statement:

// how  to avoid this?
if (allset) {
    throw new IllegalStateException("All the maps are already set");
}

Will it still work?

\$\endgroup\$
2
\$\begingroup\$

A simpler approach

As far as I can tell from your problem definition, you're overthinking it:

  • ConcurrentMap is overkill because you won't be writing concurrently. As long as you pass a memory barrier of some sort, your other threads will read correctly.

  • MapContainer doesn't seem to be serving a purpose, nor does MapContainer[] containers.

Here is a more bare-bones version of your code. (A slightly more cleaned up version is further below.)

import java.util.Map;

public class ClientData {
    private static volatile Map<String, Map<Integer, String>> mappings[] = new Map[] {
        Collections.emptyMap(),
        Collections.emptyMap(),
        Collections.emptyMap()
    };

    public static void setMappings(
            Map<String, Map<Integer, String>> primary,
            Map<String, Map<Integer, String>> secondary,
            Map<String, Map<Integer, String>> tertiary) {
        // build the array, then assign to volatile reference = memory barrier
        mappings = new Map[] {
            primary, secondary, tertiary
        };
    }

    // all these read from volatile reference = memory barrier
    public static Map<String, Map<Integer, String>>[] getMappings() {
        return mappings.clone(); // clone not strictly necessary,
                                 // but protects from accidental changes
    }

    public static Map<String, Map<Integer, String>> getPrimaryMapping() {
        return mappings[0];
    }

    public static Map<String, Map<Integer, String>> getSecondaryMapping() {
        return mappings[1];
    }

    public static Map<String, Map<Integer, String>> getTertiaryMapping() {
        return mappings[2];
    }
}

It should meet your needs:

  • Reader threads will not see incomplete changes if not all changes are already set. All your maps are published simultaneously.

  • It solves your "already set" problem.

  • Bonus: look, ma, no locks!


Why this works

Where's the synchronized? Where are the locks, Lebowski? This can't be safe, right?

Turns out that it is, since volatile semantics were changed in Java 5. From Brian Goetz:

Under the new memory model, when thread A writes to a volatile variable V, and thread B reads from V, any variable values that were visible to A at the time that V was written are guaranteed now to be visible to B. The result is a more useful semantics of volatile, at the cost of a somewhat higher performance penalty for accessing volatile fields.

Because mappings is a volatile reference, and there is no other reference path way to that array (aliasing), reads from and writes to it are strictly ordered. That means that you could never read the array and have incomplete writes dangling.

This is not bulletproof. In particular, if you have code that looks like this:

// Note that we go through ClientData.mappings twice
String cafeBar = getPrimaryMapping().get("bar").get(0xCAFE);
String fooBeef = getTertiaryMapping().get("foo").get(0xBEEF);

...you have no guarantee that those two variables were drawn from the same set of mappings. Another thread may have switched mappings in between.

What you do to fix this, is fetch and store the array locally for as long as you can both: (1) not tolerate interference, and (2) can tolerate stale data.

// save a reference, safe for multiple reads
Map<String, Map<Integer, String>> mappings[] = ClientData.getMappings();
String cafeBar = mappings[0].get("bar").get(0xCAFE);
String fooBeef = mappings[2].get("foo").get(0xBEEF);

In that vein, we can make the code safer to use, or at least not accidentally mess up:

public class ClientData {
    private static volatile ClientData global = new ClientData();

    // final guarantees written before publish
    private final Map<String, Map<Integer, String>> primary;
    private final Map<String, Map<Integer, String>> secondary;
    private final Map<String, Map<Integer, String>> tertiary;

    private ClientData() {
      this(Collections.emptyMap(), Collections.emptyMap(), Collections.emptyMap());
    }

    private ClientData(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;
    }

    public static void set(
            Map<String, Map<Integer, String>> primary,
            Map<String, Map<Integer, String>> secondary,
            Map<String, Map<Integer, String>> tertiary) {
        global = new ClientData(primary, secondary, tertiary);
    }

    public static ClientData get() {
        return global;
    }

    public Map<String, Map<Integer, String>> getPrimaryMapping() {
        return primary;
    }

    public Map<String, Map<Integer, String>> getSecondaryMapping() {
        return secondary;
    }

    public Map<String, Map<Integer, String>> getTertiaryMapping() {
        return tertiary;
    }
}

No more shady arrays, magic indices, and no alluring get...Mapping() to use instead of storing ClientData.get() into a local variable first.


Concurrent or Immutable?

Concurrent maps will allow you to do concurrent reads and updates without jeopardising the integrity of your map. This is critical if your mappings will be modified after they are published to other threads. It doesn't look like that's the case in your program.

Immutable maps do not allow changes and are inherently thread-safe... provided they are published in a safe manner (such as through locks, synchronized, or volatile). They are a step up from Collections.synchronizedMap in that they take a copy of the map, which will protect you from aliasing bugs. The Guava library has useful implementations of immutable maps.

\$\endgroup\$
  • \$\begingroup\$ I have similar question for review here as well. Can you please take a look whenever you have some time? I am stuck on that for a while. \$\endgroup\$ – arsenal Sep 3 '14 at 4:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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