[infinispan-dev] findbugs found issue

[David M. Lloyd]

On 09/03/2009 09:38 AM, David M. Lloyd wrote:
> On 09/03/2009 03:55 AM, Mircea Markus wrote:
>> Hi,
>>
>> findbugs detects a problem in the following code:
>>
>>    public void clear() {
>>       // This is expensive...
>>       // lock all segments
>>       for (Segment s : segments) s.lock();
>>       try {
>>          for (Segment s : segments) s.locklessClear();
>>          initLinks();
>>       } finally {
>>          for (Segment s : segments) s.unlock();
>>       }
>>    }
>> In my eyes the code is okay, am I missing something?
> 
> Yes - if one of the unlocks fail, the subsequent locks will not be 
> released.  If one of the locks fail, none of the acquired locks will be 
> released.

Also you're unlocking in the same order which you lock, which is begging 
for deadlocks.

A safer approach would be to use recursion:

private void clear(Iterator<Segment> i) {
     if (i.hasNext()) {
         Segment s = i.next();
         s.lock();
         try {
             clear(i);
         } finally {
             s.unlock();
         }
     } else {
         for (Segment s : segments) s.locklessClear();
         initLinks();
     }
}

This ensures that (a) the locks are released in the reverse order they were 
acquired in, and (b) if any lock or unlock operation fails, things don't go 
haywire.  Of course if you have hundreds or thousands of segments, you risk 
blowing out the stack.

- DML