[infinispan-dev] Per-entry lock container

[Jason Greene]

On Oct 17, 2012, at 8:30 AM, Manik Surtani <manik at jboss.org> wrote:

> 
> On 17 Oct 2012, at 14:18, Dan Berindei <dan.berindei at gmail.com> wrote:
> 
>> 
>> On Wed, Oct 17, 2012 at 3:51 PM, Manik Surtani <manik at jboss.org> wrote:
>> The problem is that I have 2 code paths:
>> 
>> 1.  Acquiring a lock.
>> 1.1. Check CHM for lock
>> 1.2. If lock == null, create new lock, do a putIfAbsent on CHM
>> 1.3. acquire the lock
>> 1.4. Check (in a loop) if the lock we have acquired is the same as the lock in the CHM, if not, release and retry.
>> 
>> 
>> I assume the retry goes back to 1.2?
>>  
>> 
>> 2.  Releasing the lock.
>> 2.1.  Get lock from CHM
>> 2.2.  Release lock.
>> 2.3.  Remove lock from CHM
>> 
>> 
>> I believe you need to remove the lock from the CHM before you release the lock, otherwise a thread can acquire the lock before you've removed it. The release should look something like this:
>> 
>> 2.1 Get the lock from CHM
>> 2.2. Check if the current thread is the owner, if not throw exception
>> 2.3. Remove lock from CHM
>> 2.4. Release lock
>> 
> 
> Right.  But instead I have this wrapped in a computeIfPresent operation.
> 
>> It's not very fair, because threads that try to lock this key after we have removed the lock from the CHM have an advantage compared to threads that have been waiting for a while on our lock and now have to acquire the lock, unlock, and try again to lock on a new key. This putIfAbsent+lock+unlock loop may hurt performance in high contention cases as well, and using a reference counting scheme would avoid it in most cases.
>> 
>> 
>> The problem is that we may have several competing threads in code path 1.  Imagine T1, waiting on 1.3., and T2, who owns the lock, releasing it in 2.2.  With some unfortunate timing, I have seen:
>> 
>> * T1 acquires the lock (1.3), does checks (1.4) and leaves code path 1.
>> * T2 removes the lock from the CHM (2.3)
>> * T3 comes in to code path 1, sees the lock missing from the CHM, creates a new lock acquires it, etc.
>> * T1 now tries to unlock the lock it thinks it owns.  Finds a different lock instance in the CHM.  All sorts of problems by this stage.
>> 
>> I have a working branch where I solved this by:
>> 
>> * Replacing the putIfAbsent in 1.2 with a compute() closure, which means the null check and store of the value is atomic wrt. any other modification on the same key. (with pIA, the null check didn't seem to be atomic?!)
>> * Replacing 2.1, 2.2 and 2.3 with a computeIfPresent() to release the lock and remove.
>> 
>> This seems to have solved things, because step 1.4 cannot proceed until any remove operation completes (competing for the same entry space in the map) and process 1 cannot get beyond 1.3 since the lock is still held by the releasing thread in step 2.  But I'm still testing further in case of edge cases.
>> 
>> 
>> Not sure about the safety of the computeIfAbsent/computeIfPresent approach, as I don't have any experience with it, but doesn't CHMV8 use unsafe operations that prevent us from using in SecurityManager scenarios?
> 
> I'll have to check.

It does use Unsafe. This isn't really a security manager problem though because you just need the permission to access a protected field, which we usually always need anyway. The bigger problem is portability, although all known JDKs have these Unsafe classes, because they all use the same concurrent impl code. That said, you can be 100% sure by simply making a version that uses AtomicFieldUpdaterXXX, extending AtomicXXX, and using other constructs and using that as the fall back version. That's what I do anyway :)