ReentrantLock - unlock() method doesn't seem to work with lists

Question

I'm writing a program that has to modify a List in two ways. Although this implementation works perfectly, it fails to let the second thread acquire the lock:

Node head = new Node(new Object(), null);
public static ReentrantLock lock = new ReentrantLock();
...

boolean add(Object o){
    lock.lock();
    Node current = head;
    Node previous = head.next;

    if(head.next==null){
        head.addNext(new Node(o,null));
        return true;
    }
    while(!(current.next == null)){
        current=current.next;
        previous=previous.next;
    }
    current.addNext(new Node(o,null));
    lock.unlock();
    return true;    
}

Maybe someone knows why is that?

Seems strange to have what appears to be a linked list with a length field... — pjp, Nov 01 '13 at 21:08
Since obtaining a lock is much slower than the operations you are performing, the code will be faster and simpler if you only have one lock per list, not per node. — Peter Lawrey, Nov 01 '13 at 21:13
And that's another of my mistakes, thanks for pointing that out. — Marta Panuszewska, Nov 01 '13 at 21:13
@PeterLawrey Why would locking X times on X different locks (never at the same time) be slower than locking X times on 1 Lock (also never at the same time)? — rolfl, Nov 01 '13 at 21:15
@rolfl I wouldn't obtain the same lock many times, but obtaining one lock many time is still faster than many locks as you get good memory access locality. I would just obtain one lock once for the entire operation. — Peter Lawrey, Nov 01 '13 at 21:18
@PeterLawrey - I'm not convinced... Having multiple locks (one per node) allows better concurrency for list modification (assuming modifications happen throughout the list's span - not just at the end or beginning) and locality makes no sense because you are having to modify things adjacent to the lock anyway (you are modifying the next/previous so you will likely have the lock as well). Your statement seems very broad/generic, and counter-intuitive to the example at hand (although I am not saying you are wrong, I just would like better information than what you provided). — rolfl, Nov 01 '13 at 21:23
@rolfl By the time you obtain two lock which can place anywhere in memory for every node, any notion that concurrency will be faster is lost. The locks themselves can be an order of magnitude more expensive than the list operation so you would need to have at least a dozen cpus actively using this list and doing nothing else to see similar performance to just locking the list once. — Peter Lawrey, Nov 01 '13 at 21:26

score 7 · Accepted Answer · answered Nov 01 '13 at 21:05

There are branches of code that will never allow the unlock to be invoked. For instance in add

if(head.next==null){
    head.addNext(new Node(o,null));
    return true;
}

You return without unlocking. You should follow the lock try finally unlock semantics.

lock.lock();
try{
   ... do stuff 
   return true;
}finally{
   lock.unlock();
}

ReentrantLock - unlock() method doesn't seem to work with lists

1 Answers1