[infinispan-dev] AtomicMapCache in transactional mode returning null

[Galder Zamarreno]

See below:

----- "Manik Surtani" <manik at jboss.org> wrote:

> On 4 May 2010, at 16:00, Galder Zamarreno wrote:
> 
> > Digging further, I discovered that this issue does not affect
> replication. It only affects cache stores.
> > 
> > This is because when replicating, TransactionXaAdapter keeps
> modifications as commands executed, so the situation below, if applied
> to a replicated environment, would result on the two modifications
> pointing to different atomic hash map instances. So, the effects of
> the sending the first atomic hash map would have no effect on the
> second.
> > 
> > However, this is not the same with cache stores. Here,
> StoreModificationsBuilder comes into play which transforms those
> commands into loaders.modifications.Modification built with data
> inspected from the cache. Hence, those two commands end up with two
> modifications that have retrieved the same atomic hash map from the
> cache. Hence, the effects of replicating the first are seen by the
> second modification. This ultimately leads to the issue below.
> > 
> > Regardless, I still think that there shouldn't be multiple
> modifications/commands for the same thing here. It's wasteful for a
> start and has secondary effects as shown below.
> 
> Good point and thanks for the detailed analysis.  Could you add this
> as a test?  Further, does this happen when we use stores + repl/dist,
> or just stores + local mode?

I added a test to BaseCacheStoreFunctionalTest so that atomic hash maps are tested. The issue goes when putIfAbsent is removed from AtomicHashMap.newInstance method call. However, as you hinted already, even with this fix, the same test fails when when you have a store *and* repl/dist. This is because the same AtomicHashMap is marshalled twice, once for replication and once storing in cache store. Hence, the 2nd time null delta is written. I'm looking into improving this.

> 
> Cheers
> Manik
> 
> > 
> > Cheers,
> > 
> > ----- galder at redhat.com wrote:
> > 
> >> 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
> >> 
> >> _______________________________________________
> >> infinispan-dev mailing list
> >> infinispan-dev at lists.jboss.org
> >> https://lists.jboss.org/mailman/listinfo/infinispan-dev
> > 
> > _______________________________________________
> > infinispan-dev mailing list
> > infinispan-dev at lists.jboss.org
> > https://lists.jboss.org/mailman/listinfo/infinispan-dev
> 
> --
> Manik Surtani
> manik at jboss.org
> Lead, Infinispan
> Lead, JBoss Cache
> http://www.infinispan.org
> http://www.jbosscache.org
> 
> 
> 
> 
> 
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev