Findbugs and locks?

Thread view:

Knute Johnson - 30 Jul 2008 06:06 GMT

Findbugs gives the warning "Method does not release lock on all
exception paths" on a method like the one below. Could it be because
the lock is from an array of locks and it can't determine which? Or is
it because you could put code outside of the try/finally block that
could leave without unlocking the lock? Any other ideas? It can't
leave the method without unlocking can it?

ReentrantReadWriteLock lock[] = new ReentrantReadWriteLock[5];
for (int i=0; i<lock.length; i++)
lock[i] = new ReentrantReadWriteLock();

static void method(int n) throws IOException {
lock[n].writeLock().lock();
try {
// do some disk I/O
} finally {
lock[n].writeLock().unlock();
}
}

Thanks,

Signature

Knute Johnson
email s/nospam/knute2008/

Reply to this Message

Jeff Higgins - 30 Jul 2008 13:46 GMT

> Findbugs gives the warning "Method does not release lock on all
> exception paths" on a method like the one below. Could it be because
> the lock is from an array of locks and it can't determine which? Or is
> it because you could put code outside of the try/finally block that
> could leave without unlocking the lock? Any other ideas? It can't
> leave the method without unlocking can it?

Produces no bug reports using FindBugs 1.3.2.20080222
in Eclipse 3.3

import java.io.IOException;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock.WriteLock;

public class SSCCE {

ReentrantReadWriteLock lockArray[] =
new ReentrantReadWriteLock[5];

void method(int n) throws IOException {

if (n < lockArray.length) {
ReentrantReadWriteLock rrwl = lockArray[n];
WriteLock lock = rrwl.writeLock();
try {
lock.lock();
// do some disk I/O
} finally {
lock.unlock();
}
}
}

public static void main(String[] args) {

}

}

Reply to this Message

Lew - 30 Jul 2008 14:17 GMT

> if (n < lockArray.length) {
> ReentrantReadWriteLock rrwl = lockArray[n];
[quoted text clipped - 7 lines]
> }
> }

Is there a chance that lock.lock() would fail?

If so, would lock.unlock() cause any harm?

The usual pattern for resource acquisition and release with try-finally is

resource.acquire();
// break flow on acquire() failure
try
{
...
}
finally
{
resource.release();
}

That way the release() only occurs if the acquire() succeeds. Moving the
acquire() into the try{} means that the release() will occur even if acquire()
fails. This is how Knute coded it, and I don't see why his way caused a warning.

I would be more comfortable if Knute's way didn't cause any warnings, because
I don't understand it either. However, I'm unfamiliar with Findbugs (although
many recommend it very highly).

Signature

Lew

Reply to this Message

Jeff Higgins - 30 Jul 2008 14:59 GMT

>> if (n < lockArray.length) {
>> ReentrantReadWriteLock rrwl = lockArray[n];
[quoted text clipped - 29 lines]
> acquire() fails. This is how Knute coded it, and I don't see why his
> way caused a warning.

Well, double oops then. You're right, I'm too quick with the send button.

Still produces no bug reports.

import java.io.IOException;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock.WriteLock;

public class SSCCE {

static ReentrantReadWriteLock lockArray[];
static {
lockArray =
new ReentrantReadWriteLock[5];
for (int i=0; i<lockArray.length; i++)
lockArray[i] = new ReentrantReadWriteLock();
}

static void method(int n) throws IOException {

WriteLock lock = lockArray[n].writeLock();
lock.lock();
try {
// do some disk I/O
} finally {
lock.unlock();
}
}

public static void main(String[] args) {}
}

> I would be more comfortable if Knute's way didn't cause any warnings,
> because I don't understand it either. However, I'm unfamiliar with
> Findbugs (although many recommend it very highly).

Reply to this Message

Jeff Higgins - 30 Jul 2008 14:28 GMT

Oops.
Still no bug reports.

import java.io.IOException;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock.WriteLock;

public class SSCCE {

static ReentrantReadWriteLock lockArray[];
static {
lockArray =
new ReentrantReadWriteLock[5];
for (int i=0; i<lockArray.length; i++)
lockArray[i] = new ReentrantReadWriteLock();
}

static void method(int n) throws IOException {

if (n < lockArray.length) {
WriteLock lock = lockArray[n].writeLock();
try {
lock.lock();
// do some disk I/O
} finally {
lock.unlock();
}
}
}

public static void main(String[] args) {

}

}

Reply to this Message

Knute Johnson - 30 Jul 2008 18:41 GMT

>> Findbugs gives the warning "Method does not release lock on all
>> exception paths" on a method like the one below. Could it be because
[quoted text clipped - 34 lines]
>
> }

Putting the lock() call inside the try/finally block does stop findbugs
from complaining. I put it on the outside because that is the way that
Goetz showed in his book, Java Concurrency In Practice. He does mention
that you must consider what happens if an exception is thrown outside of
the try block. I suppose findbugs complains about my code is it is
possible to throw an exception between the lock and the try even though
I have no code there.

I don't know for sure what the ramifications are of putting the lock
inside the try block but I can't think of any at the moment.

Thanks,

Signature

Knute Johnson
email s/nospam/knute2008/

Reply to this Message

Jeff Higgins - 30 Jul 2008 18:59 GMT

>>> Findbugs gives the warning "Method does not release lock on all
>>> exception paths" on a method like the one below. Could it be because
[quoted text clipped - 45 lines]
> I don't know for sure what the ramifications are of putting the lock
> inside the try block but I can't think of any at the moment.

Sorry for the confusion, I too quickly posted an experimental version.
I can't figure out what exception path the following might close, but
FindBugs does not produce a bug report for the following:

import java.io.IOException;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock.WriteLock;

public class SSCCE {

static ReentrantReadWriteLock lockArray[];
static {
lockArray =
new ReentrantReadWriteLock[5];
for (int i=0; i<lockArray.length; i++)
lockArray[i] = new ReentrantReadWriteLock();
}

static void method(int n) throws IOException {

WriteLock lock = lockArray[n].writeLock();
lock.lock();
try {
// do some disk I/O
} finally {
lock.unlock();
}
}

public static void main(String[] args) {}
}

Reply to this Message

Knute Johnson - 30 Jul 2008 20:33 GMT

> Sorry for the confusion, I too quickly posted an experimental version.
> I can't figure out what exception path the following might close, but
[quoted text clipped - 27 lines]
> public static void main(String[] args) {}
> }

Ah but look at what does!

static void method(int n) throws IOException {

// WriteLock lock = lockArray[n].writeLock();
// lock.lock();
lockArray[n].writeLock().lock();
try {
// do some disk I/O
} finally {
// lock.unlock();
lockArray[n].writeLock().unlock();
}
}

I think it is a bug in findbugs. I think I'll drop them a line.

Thanks,

Signature

Knute Johnson
email s/nospam/knute2008/

Reply to this Message

Daniel Pitts - 31 Jul 2008 01:41 GMT

> Ah but look at what does!
>
[quoted text clipped - 12 lines]
>
> I think it is a bug in findbugs. I think I'll drop them a line.

I don't think its a bug in findbugs.
The problem is that lockArray[n] might through an exception in the
finally if the lockArray changes size. lockArray[n] might also be
assigned a null at some point, so lockArray[n].writeLock() could through
an NPE.

I think a better approach would be to have method take a Lock rather
than an index into an array.

Hope this helps,
Daniel.

Signature

Daniel Pitts' Tech Blog: <http://virtualinfinity.net/wordpress/>

Reply to this Message

Knute Johnson - 31 Jul 2008 06:49 GMT

>> Ah but look at what does!
>>
[quoted text clipped - 24 lines]
> Hope this helps,
> Daniel.

If the lock throws an exception, how can it be locked?

Signature

Knute Johnson
email s/nospam/knute2008/

Reply to this Message

Jeff Higgins - 31 Jul 2008 12:14 GMT

>>> Ah but look at what does!
>>>
[quoted text clipped - 26 lines]
>
> If the lock throws an exception, how can it be locked?

After a night's rest, I think that:
ReentrantReadWriteLock.writeLock() is a factory method.

import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock.WriteLock;

public class SSCCE {

ReentrantReadWriteLock reentrantLock =
new ReentrantReadWriteLock();

void methodA() {

// FindBugs 1.3.2.2008022 Eclipse 3.3.1.1 WinXP
//produces no bug report
WriteLock lock = reentrantLock.writeLock();
lock.lock();
try {
// do some disk I/O
} finally {
// Here we have a reference to _lock_ WriteLock.
lock.unlock();
}
}

void methodB() {

// FindBugs 1.3.2.2008022 Eclipse 3.3.1.1 WinXP produces:
// [UL] Method does not release lock on all exception paths
// [UL_UNRELEASED_LOCK_EXCEPTION_PATH]
// on the following line
reentrantLock.writeLock().lock();
try {
// do some disk I/O
} finally {
// Here we unlock _any_ WriteLock that
// ReentrantReadWriteLock.writeLock() constructs.
reentrantLock.writeLock().unlock();
}
}

public static void main(String[] args) {}
}

Reply to this Message

Jeff Higgins - 31 Jul 2008 12:39 GMT

>>>> Ah but look at what does!

> After a night's rest, I think that:
> ReentrantReadWriteLock.writeLock() is a factory method.

Oops, getting my coffee now.

Reply to this Message

Findbugs and locks?

Free Magazines