[infinispan-dev] AtomicMapCache in transactional mode returning null

[galder at redhat.com]

Hi,

This is related to https://community.jboss.org/message/541023#541023 thread. Once https://jira.jboss.org/jira/browse/ISPN-418 fails, another issue appears whereby a null delta is written to the cache store. The reason for this is that the code in testRestoreTransactionalAtomicMap actually generates two modifications in the transaction. 

The first one comes from the putIfAbsent below that can be found in AtomicHashMap:

   public static AtomicHashMap newInstance(Cache cache, Object cacheKey) {
      AtomicHashMap value = new AtomicHashMap();
      Object oldValue = cache.putIfAbsent(cacheKey, value);
      if (oldValue != null) value = (AtomicHashMap) oldValue;
      return value;
   }

And the second modification comes from AtomicHashMapProxy.getDeltaMapForWrite call by the code below. This comes from the map put call:

   public V put(K key, V value) {
      try {
         startAtomic();
         InvocationContext ctx = icc.createInvocationContext();
         AtomicHashMap<K, V> deltaMapForWrite = getDeltaMapForWrite(ctx);
         return deltaMapForWrite.put(key, value);
      }
      finally {
         endAtomic();
      }
   }

On one side, these two modifications do exactly the same, a put with the atomic hash map, so it's not very efficient. One put would be enough for the test to work. However, the reason the test fails is after writing the first modification via:

      public void writeObject(ObjectOutput output, Object subject) throws IOException {
         DeltaAware dw = (DeltaAware) subject;
         output.writeObject(dw.delta());
      }

dw.delta() call resets the delta and sets it to null. So, when the second modification is written, delta is null and that's the final value written. Obviously, when data is read, null delta is retrieved and hence the test fails. 

So IMO, one of the two modifications must go and my vote is for the one in newInstance. I don't really understand why we should put when creating a new instance of AtomicHashMap. It makes more sense to do the put when you actually modify the map via a put call for example. Manik, any idea why you added the putIfAbsent to newInstance?

As a side note, and this might be related, the javadoc of newInstance needs updating since AtomicMapCache does not exist. It should probably refer to AtomicMapLookup instead of AtomicMapCache.

   /**
    * Construction only allowed through this factory method.  This factory is intended for use internally by the
    * CacheDelegate.  User code should use {@link org.infinispan.atomic.AtomicMapCache#getAtomicMap(Object)}.
    */

Cheers,
--
Galder Zamarreño
Sr. Software Engineer
Infinispan, JBoss Cache