Concurrently reading a Map while a single background thread regularly modifies it

I have a class in which I am populating a map liveSocketsByDatacenter from a single background thread every 30 seconds inside updateLiveSockets() method and then I have a method getNextSocket() which will be called by multiple reader threads to get a live socket available which uses the same map to get this info.

```
public class SocketManager {
private static final Random random = new Random();
private final ScheduledExecutorService scheduler = Executors.newSingleThreadScheduledExecutor();
private final AtomicReference>> liveSocketsByDatacenter =
new AtomicReference<>(Collections.unmodifiableMap(new HashMap<>()));
private final ZContext ctx = new ZContext();

// Lazy Loaded Singleton Pattern
private static class Holder {
private static final SocketManager instance = new SocketManager();
}

public static SocketManager getInstance() {
return Holder.instance;
}

private SocketManager() {
connectToZMQSockets();
scheduler.scheduleAtFixedRate(new Runnable() {
public void run() {
updateLiveSockets();
}
}, 30, 30, TimeUnit.SECONDS);
}

private void connectToZMQSockets() {
Map> socketsByDatacenter = Utils.SERVERS;
// The map in which I put all the live sockets
Map> updatedLiveSocketsByDatacenter = new HashMap<>();
for (Map.Entry> entry : socketsByDatacenter.entrySet()) {
List addedColoSockets = connect(entry.getKey(), entry.getValue(), ZMQ.PUSH);
updatedLiveSocketsByDatacenter.put(entry.getKey(),
Collections.unmodifiableList(addedColoSockets));
}
// Update the map content
this.liveSocketsByDatacenter.set(Collections.unmodifiableMap(updatedLiveSocketsByDatacenter));
}

private List connect(Datacenters colo, List addresses, int socketType) {
List socketList = new ArrayList<>();
for (String address : addresses) {
try {
Socket client = ctx.createSocket(socketType);
// Set random identity to make tracing easier
S

In general, the concept you are trying to accomplish is appropriate for the task at hand, a lazy build of the live sockets, followed by a quick "switch" of what the clients can see.

Using an AtomicReference is also what I would do to accomplish the core thread-safe switch.

In general, the approach you take is a good one.

Some of the implementation details are not as ideal, or modern, as I would expect, and there are some tricks with Atomics that you're not using.... which would help.

Singletons

The singleton you have for your SocketManager is reasonable, but the best practice is to use an enum to solve it. Your code has:

// Lazy Loaded Singleton Pattern
  private static class Holder {
    private static final SocketManager instance = new SocketManager();
  }

  public static SocketManager getInstance() {
    return Holder.instance;
  }

but this should really be:

// Lazy Loaded Singleton Pattern
  private enum Holder {
      INSTANCE;
      private final SocketManager manager = new SocketManager();
  }

  public static SocketManager getInstance() {
    return Holder.INSTANCE.manager;
  }

There are a number of advantages to using an enum for the singleton pattern...

Joshua Bloch writes: "This approach is functionally equivalent to the public field approach, except that it is more concise, provides the serialization machinery for free, and provides an ironclad guarantee against multiple instantiation, even in the face of sophisticated serialization or reflection attacks. While this approach has yet to be widely adopted, a single-element enum type is the best way to implement a singleton."

Implementing getNextSocket()

Your code implementing this method has a number of 2nd-tier issues (there are no functionality/bug/edge-case issues I can see, just code-style and best-practice issues), some are hard to describe.

First up, just to remind myself, and others, this is a multi-thread entry point - this method can be called concurrently by many threads. Your implementation is such that all actions on the "live" socket map should be read-only, and thus there are no mutations to the map that we need to worry about... and we know that the update thread does not change THIS map, it creates a new map, and at some point swaps that new map in. So, in this method, as soon as we have retrieved the map from the atomic reference, we can do what we like (as long as it is read-only), and not have concurrency issues.

Right, your code correctly does all the above.... good. There's no problems I can see in that regard.

Also, I really like that you are returning an Optional result, it's a good strategy.

But.... let's look at some of the issues, starting with "early returns". It is common, and simpler in Java to have an early-return, or a nested return, than it is to have a "placeholder" variable. In addition, you can iterate on the ordered datacenters directly without the variable. Further, while I do appreciate that your variable names are very descriptive, you can find a compromise between too short, and too long. I would reduce the "verbose-ness" of variable names especially when they are private and in a small scope like a small function. Similarly I would remove this references as they are mostly redundant. In your code you have:

Map> liveSocketsByDatacenter =
        this.liveSocketsByDatacenter.get();
    Optional liveSocket = Optional.absent();
    List dcs = Datacenters.getOrderedDatacenters();
    for (Datacenters dc : dcs) {
      liveSocket = getLiveSocket(liveSocketsByDatacenter.get(dc));
      if (liveSocket.isPresent()) {
        break;
      }
    }
    return liveSocket;

but that would be simpler if you got rid of the liveSocket variable completely, and have some shorter variable names, and had just:

Map> liveByDC = liveSocketsByDatacenter.get();
for (Datacenters dc : Datacenters.getOrderedDatacenters()) {
  Optional socket = getLiveSocket(liveByDC.get(dc));
  if (socket.isPresent()) {
    return socket;
  }
}
return Optional.absent();

Implementing getLiveSocket()

Similar to the calling function, this one is accessed concurrently, and again, I like the use of Optional, etc.

Where this function has problems is primarily the way you copy and shuffle the data. It's overkill, and inefficient. You also have similar issues with overly long variable names, etc.

Oddly, in this method, you use the early-return concept really well.

What you should be using instead, in here, is the concept of a "Guard Clause". Instead of checking for a non-empty list (using CollectionsUtils - whatever that is - is overkill too) you should just check for an empty list and return.

Your method is:

```
private Optional getLiveSocket(final List listOfEndPoints) {
if (!CollectionUtils.isEmpty(listOfEndPoints)) {
// The list of live sockets
List liveOnly = new ArrayList<>(listOfEndPoints.size());
for (SocketHolder obj : listOfEndPoints) {
if (obj.

StackExchange Code Review Q#156917, answer score: 2