Is this nested map structure safe for multi-thread reads?

Question

I already searched through some SO questions like this one, and this one, but no real conclusion could be made on my end. So, I'll lay it out:

I have the following nested map structure concept, intended for multi-threaded environment:

Map<Integer, HashMap<String, AtomicInteger>> bufferMap = new ConcurrentHashMap<Integer, HashMap<String, AtomicInteger>>(2);

This "buffer map" should basically stores some hourly counters (AtomicIntegers), identified / accessed by specific String keys. So, Integer keys for the buffer map are actually hour numbers (0...23). I only ever intend to "buffer" current and pre-set the next hour. To that effect, there's a timer task that runs once an hour, and does a maintaining procedure - something like this:

private final Map<Integer, HashMap<String, AtomicInteger>> bufferMap;
...
private final java.util.Timer;
private final java.util.TimerTask task = new TimerTask() {  
@Override
public void run() {
    ....
    HashMap<String, AtomicInteger> counterMap = bufferMap.get(previousHour);

    // now read internalMap's values, and "store/flush" them somewhere
    // at this point no thread but this one should access previous hour data

    initializeNextHourSlot(); // populate new map entry for the next hour with new AtomicInteger(0) values

    bufferMap.remove(previousHour); // clear previous hour, as no longer needed
}
}

Now, multiple threads may randomly and/or in parallel access this structure, to increment counters in the following manner:

bufferMap.get(currentHour).get(stringKey).incrementAndGet();

As outer (buffer) map is actually modified by different (Timer's) thread than the ones reading it, it was logical, I assume, to use ConcurrentHashMap.

However, I have my doubts about the inner (counter) map... It will always be populated by the timer thread ahead of time (no other threads should access it for at least an hour), and will then be accessed (read only), as shown above, to increment counter values.

Is this a thread safe approach, or not? And if not, what could be an alternative suggested data structure (and/or approach) ?

Answer 1

Is this a thread safe approach, or not?

You can't be sure when a thread will run. Even if you create the structure an hour before you need it, the process could be placed in hibernation and still fail to run (in theory)

And if not, what could be an alternative suggested data structure (and/or approach) ?

A simpler approach is to not use a timer but a compute if absent.

final AtomicReference<TimedData> ref = new AtomicReference<>();

public void increment(String counter) {
   TimedData td = ref.get();
   long hour = System.currentTimeMillis() / 3_600_000;
   if (td.hour != hour) {
       saveData(td); // use back ground thread if needed.
       if (!ref.compareAndSet(td, new TimedData(hour))
            td = ref.get();
   }

   td.counterMap.get(counter)
                .incrementAndGet();
}

for the class

class TimedData {
    final long hour;
    final Map<String, AtomicInteger> counterMap = new HashMap<>();

    public TimedData(long hour) {
        this.hour = hour;
        // init the counterMap
    }

In this case, the background thread is optional and it doesn't matter when it runs.

Answer 2

First off, this is not safe. Your timer and writers may concurrently read/write the current hour data. Consider that the line:

bufferMap.get(currentHour).get(stringKey).incrementAndGet();

Is not atomic and this means you actually have:

hourlyMap = bufferMap.get(currentHour);
// assume this thread was now suspended by the OS for some time
keyCounter = hourlyMap.get(stringKey);
keyCounter.incrementAndGet();

It seems to me what you are after is having 2 maps that the timer thread can swap between, but apart from efficiency this is not helpful to the problem above. Assuming you are looking to have a coherent snapshot of all counters (which is a strong requirement) you'll need to exclude writers from updating the counters while you read. There's an example of solving this exact problem in the HdrHistogram library Recorder class using a PhaseLock (http://hdrhistogram.org/), to hack in your map of counters:

public void incKey(String k) {
    long criticalValueAtEnter = recordingPhaser.writerCriticalSectionEnter();
    try {
        activeCounterMap.get(k).incrementAndGet();
    } finally {
        recordingPhaser.writerCriticalSectionExit(criticalValueAtEnter);
    }
}

private void sampleCounters() {
    try {
        recordingPhaser.readerLock();
        // ...swap your maps here...
        recordingPhaser.flipPhase(500000L /* yield in 0.5 msec units if needed */);
    } finally {
        recordingPhaser.readerUnlock();
    }
}

I believe there are other similar locks you can use to acheive the same effect. What you want is many writers to be prioritized over single reader I assume.