HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Lock for preventing concurrent access in client data

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
dataclientforpreventingconcurrentaccesslock

Problem

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

Here is my ClientData class in which I am using CountDownLatch:

public class ClientData {

    private static final AtomicReference>> primaryMapping = new AtomicReference<>();
    private static final AtomicReference>> secondaryMapping = new AtomicReference<>();
    private static final AtomicReference>> tertiaryMapping = new AtomicReference<>();

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

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

        return primaryMapping.get();
    }

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

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

        return secondaryMapping.get();
    }       

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

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

        return tertiaryMapping.get();
    }       

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


I need to wait on the get calls on three AtomicReferences I have. Once all the writes has been done on the three AtomicReferences I have with the set call, I'll allow making the call to three getters which I have.

I decided

Solution

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> primary;
        public final Map> secondary;
        public final Map> tertiary;

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

    private static final AtomicReference 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> primary,
        Map> secondary,
        Map> 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.

Code Snippets

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

Context

StackExchange Code Review Q#48442, answer score: 7

Revisions (0)

No revisions yet.