CODEWITHSUNDEEP
javafindbugsjava.util.concurrent
Geek
Geek

Reputation: 27203

Probable false positive from Findbugs?

Please refer to the following code snippet(stripped off the redundant part to highlight the problematic case):

FindBugs is complaining that "Method does not release lock on all paths" . Is this a false positive? If not, how to fix this?

  try{
      someLock.lock();
     //do something
    } finally{
      if (someLock.isLocked())
        someLock.unlock();
    }

Upvotes: 1

Views: 1282

Answers (3)

Denys Séguret
Denys Séguret

Reputation: 382264

If isLocked() throws something, then you don't unlock.

I don't think it's probable to have isLocked throwing an exception but when you lock, you must unlock, there is no point in testing. So why not use the standard pattern described in the javadoc :

someLock.lock();
try{
    //do something
} finally{
    someLock.unlock();
}

So I'd said

  • it's a false positive in the sense your code can't fail to unlock
  • it's a true positive in the sense you should fix your code according to the recommendation

Upvotes: 5

Stephen C
Stephen C

Reputation: 719189

This looks like a false positive, but I think I can understand why you get it.

The FindBugs rule is most likely coded to check that all paths call unlock ... irrespective of whether the call is actually needed (from the perspective of the lock's state). It most likely does not make any attempt to track the state of the lock, and is most likely not aware of what isLocked means. While it is obvious to you and I that it is unnecessary to call unlock if isLocked returns false, the FindBugs rule is not implemented to make this inference.

(Indeed, making the inference reliably in a wide range of use-cases would be a difficult problem for the FindBugs implementors. We are in "automatic theorem prover" territory ...)

Upvotes: 1

Javier
Javier

Reputation: 12398

isLocked

Queries if this lock is held by any thread. This method is designed for use in monitoring of the system state, not for synchronization control.

Not being "for synchronization control" means that the implementation of isLocked is not guaranteed to be free from race conditions, and it may return false even if we have acquired the lock.

someLock.lock();
try{      
 //do something
} finally{
  someLock.unlock();
}

or 

boolean locked=false;
try{      
 someLock.lock();
 locked=true;
 //do something
} finally{
  if (locked) someLock.unlock();
}

Upvotes: 3

Related Questions