-
Bug
-
Resolution: Fixed
-
P4
-
None
-
None
Before the example in the class specification of ReentrantLock, there is the text:
It is recommended practice to always immediately follow a
call to lock with a try block, most typically in a before/after
construction such as:
This is reasonable as far as it goes, but it might be worthwhile to add some details. The call to `lock()` should occur *immediately before* the beginning of the try block (but not inside of it), with no intervening statements or expressions. The call to `unlock()` should occur as the *very first* statement of the finally block.
The danger here is that somebody might put in an apparently innocuous statement (such as logging a message) that, if it were to throw an exception, would violate the locking invariants.
The example in ReentrantReadWriteLock can lead to more insidious bugs. It's not incorrect, and the general recommendations still apply but more subtly. Specifically, the code is in a precarious state between the lock acquisition and the beginning of the try-finally statement that releases it. The code shouldn't do anything that throws any exceptions, lest the lock never be released. The example is:
rwl.readLock().lock();
if (!cacheValid) {
...
}
try {
...
} finally {
rwl.readLock().unlock();
}
In this code, initial read lock acquisition is quite far from the try-finally statement that protects it. It would be very easy for errors to creep in here. For example, an apparently innocuous refactoring from checking the boolean cacheValid field to calling an isCacheValid() method could introduce errors, if that method could possibly throw an exception.
Naturally one could write volumes about this issue; that probably isn't appropriate for the class specifications in javadoc. However, some brief judicious warnings and exhortations highlighting the issue might be appropriate.
It is recommended practice to always immediately follow a
call to lock with a try block, most typically in a before/after
construction such as:
This is reasonable as far as it goes, but it might be worthwhile to add some details. The call to `lock()` should occur *immediately before* the beginning of the try block (but not inside of it), with no intervening statements or expressions. The call to `unlock()` should occur as the *very first* statement of the finally block.
The danger here is that somebody might put in an apparently innocuous statement (such as logging a message) that, if it were to throw an exception, would violate the locking invariants.
The example in ReentrantReadWriteLock can lead to more insidious bugs. It's not incorrect, and the general recommendations still apply but more subtly. Specifically, the code is in a precarious state between the lock acquisition and the beginning of the try-finally statement that releases it. The code shouldn't do anything that throws any exceptions, lest the lock never be released. The example is:
rwl.readLock().lock();
if (!cacheValid) {
...
}
try {
...
} finally {
rwl.readLock().unlock();
}
In this code, initial read lock acquisition is quite far from the try-finally statement that protects it. It would be very easy for errors to creep in here. For example, an apparently innocuous refactoring from checking the boolean cacheValid field to calling an isCacheValid() method could introduce errors, if that method could possibly throw an exception.
Naturally one could write volumes about this issue; that probably isn't appropriate for the class specifications in javadoc. However, some brief judicious warnings and exhortations highlighting the issue might be appropriate.