[infinispan-dev] code review

[Sanne Grinovero]

Hi Łukasz,
I'm still unable to reproduce your same exception, but I started to
develop an additional test
to get more thread interleaving, my original goal was to make your
problem happen more likely
but I got some other errors regarding the lock & transaction management.

Here is some early feedback, hoping to get this somewhere in SVN soon
to make it easier
to help out:

1) make sure all Logger instances are made "static", especially since the logger
factory we are using is quite expensive.

2) Using "synchronized ( cache ) " in InfinispanLock:
there's something wrong with this approach, especially here:

if ( !cache.containsKey( lock ) ) {
						cache.put( lock, lock );
						acquired = true;
					}

this is not atomical: considering at first the "synchronized (cache)"
it means you want to guarantee that nobody else uses it in a similar method,
but this guarantee holds only on the same reference of "cache", especially
only to the current node; other nodes might change the contents of the cache
and synch won't stop that, ending up with two or more nodes owning the Lucene
directory lock.

In the next step the code is asking if the lock is taken, if not you
take it and consider
yourself as owner. actually during cache.put you should check the
return value as some
other node could have acquired it.
Even better, give a look into using Infinispan's atomic API:

cache.putIfAbsent(K,V);

and again make sure you check for the return value.

In any case, I think you'll need to avoid all usages of "synchronized"
as it's not giving
the safety needed for this use case, and exploit the atomic APIs
Infinispan is providing.

cheers,
Sanne