[infinispan-dev] AtomicHashMap concurrent modifications in pessimistic mode

[Dan Berindei]

On Mon, May 20, 2013 at 1:57 PM, Manik Surtani <msurtani at redhat.com> wrote:

>
> On 16 May 2013, at 15:04, Dan Berindei <dan.berindei at gmail.com> wrote:
>
> Hi guys
>
> I'm working on an intermittent failure in NodeMoveAPIPessimisticTest and I
> think I've come across what I think is underspecified behaviour in
> AtomicHashMap.
>
> Say we have two transactions, tx1 and tx2, and they both work with the
> same atomic map in a pessimistic cache:
>
> 1. tx1: am1 = AtomicMapLookup.get(cache, key)
> 2. tx2: am2 = AtomicMapLookup.get(cache, key)
> 3. tx1: am1.put(subkey1, value1) // locks the map
> 4. tx2: am2.get(subkey1) // returns null
> 5. tx1: commit // the map is now {subkey1=value1}
> 6. tx2: am2.put(subkey2, value2) // locks the map
> 7. tx2: commit // the map is now {subkey2=value2}
>
> It's not clear to me from the AtomicMap/AtomicHashMap javadoc if this is
> ok or if it's a bug...
>
>
> If optimistic, step 7 should fail with a write skew check.  If
> pessimistic, step 2 would *usually* block assuming that another thread is
> updating the map, but since neither tx1 or tx2 has started updating the map
> yet, neither has a write lock on the map.  So that succeeds.  I'm not sure
> if this is any different from not using an atomic map:
>
> 1.  tx1: cache.get(k, v); // reads into tx context
> 2.  tx2: cache.get(k, v);
> 3.  tx1: cache.put(k, v + 1 );
> 4.  tx1: commit
> 5.  tx2: cache.put(k, v + 1 );
> 6.  tx2: commit
>
> here as well, if using optimistic, step 6 will fail with a WSC but if
> pessimistic this will work (since tx2 only requested a write lock after tx1
> committed/released its write lock).
>
>
The difference is that in your scenario, you see in the code that tx2
writes to key k, so it's not surprising to find that tx2 overwrote the
value written by tx1. But it would be surprising if tx2 also overwrote an
unrelated key k2.

With an atomic map, you only see in the code "map.put(subkey2, value)". Tx2
doesn't touch subkey1, so it's not that obvious that it should remove it.
It is clear to me why it behaves the way it does now, after reading the
implementation, but I don't think it's what most users would expect. (The
proof, I guess, is in the current implementation of TreeCache.move()).

With a FineGrainedAtomicMap in optimistic mode, it's not obvious why tx1
writing to subkey1 should cause tx2's write to subkey2 fail, either (see
https://issues.jboss.org/browse/ISPN-3123).


> Note that today the map is overwritten by tx2 even without step 4 ("tx2:
> am2.get(subkey1)"). I'm pretty sure that's a bug and I fixed it locally by
> using the FORCE_WRITE_LOCK in AtomicHashMapProxy.getDeltaMapForWrite.
>
> However, when the Tree API moves a node it first checks for the existence
> of the destination node, which means NodeMoveAPIPessimisticTest is still
> failing. I'm not sure if I should fix that by forcing a write lock for all
> AtomicHashMap reads, for all TreeCache reads, or only in TreeCache.move().
>
>
> I think only in TreeCache.move()
>
>
I tend to disagree. I think it's way too easy to introduce a read of a
node's structure in a transaction and start losing data without knowing it.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/infinispan-dev/attachments/20130520/7b2c1546/attachment.html