Is a Guava Table thread safe when its backing maps are thread safe?

Question

Will Guava's Tables.newCustomTable(Map, Supplier) method return thread safe tables when supplied with thread safe maps? For example:

public static <R, C, V> Table<R, C, V> newConcurrentTable() {
  return Tables.newCustomTable(
      new ConcurrentHashMap<R, Map<C, V>>(),
      new Supplier<Map<C, V>>() {
        public Map<C, V> get() {
          return new ConcurrentHashMap<C, V>();
        }
      });
}

Does that code actually return concurrent tables?

Answer 1

From the doc: "If multiple threads access this table concurrently and one of the threads modifies the table, it must be synchronized externally."

Concurrent backing collections aren't enough.

Answer 2

Kevin Bourrillion is right. The technical reason for the map you've constructed not to be thread safe is that even if the maps you are using are thread safe, the table operations may not be. Let me give an example of put, as implemented in the StandardTable, which is used by Tables.newCustomTable:

public V put(R rowKey, C columnKey, V value) {
  Map<C, V> map = backingMap.get(rowKey);
  if (map == null) {
    map = factory.get();
    backingMap.put(rowKey, map);
  }
  return map.put(columnKey, value);
}

Thread safety is compromised in the handling of the map == null case. Namely, two or more threads could enter that block and create a new entry for the columnKey and the last one to perform a backingMap.put(rowKey, map) would ultimately override the entry for the columnKey in the backingMap, which would lead to the loss of put operations performed by other threads. In particular the result of this operation in a multithreaded environment is non-deterministic, which is equivalent to saying that this operation is not thread safe.

The correct implementation of this method would be:

public V put(R rowKey, C columnKey, V value) {
    ConcurrentMap<C, V> map = table.get(rowKey);
    if (map == null) {
        backingMap.putIfAbsent(rowKey, factory.get());
    }
    map = backingMap.get(rowKey);
    return map.put(columnKey, value);
}

I'm currently investigating if it is possible to use the ForwardingTable implementation together with what you've wanted to do, to get a properly thread safe ConcurrentTable.

But to be honest, I think the reason there is no thread-safe implementation of the Table is that the interface itself doesn't provide any concurrency constructs, such as putIfAbsent or replace.