Multithreaded correctness - rules provided by FindBugs
Sequence of calls to concurrent abstraction may not be atomic
This code contains a sequence of calls to a concurrent abstraction (such as a concurrent hash map). These calls will not be executed atomically.
Possible double check of field
This method may contain an instance of double-checked locking.
This idiom is not correct according to the semantics of the Java memory model.
Synchronization on Boolean
The code synchronizes on a boxed primitive constant, such as an Boolean.
private static Boolean inited = Boolean.FALSE;
...
synchronized(inited) {
if (!inited) {
init();
inited = Boolean.TRUE;
}
}
...
Since there normally exist only two Boolean objects, this code could be synchronizing on the same object as other, unrelated code, leading to unresponsiveness and possible deadlock
Synchronization on boxed primitive
The code synchronizes on a boxed primitive constant, such as an Integer.
private static Integer count = 0;
...
synchronized(count) {
count++;
}
...
Since Integer objects can be cached and shared, this code could be synchronizing on the same object as other, unrelated code, leading to unresponsiveness and possible deadlock
Synchronization on interned String
The code synchronizes on interned String.
private static String LOCK = "LOCK";
...
synchronized(LOCK) { ...}
...
Constant Strings are interned and shared across all other classes loaded by the JVM.
Thus, this could is locking on something that other code might also be locking.
This could result in very strange and hard to diagnose blocking and deadlock behavior.
Synchronization on boxed primitive values
The code synchronizes on an apparently unshared boxed primitive, such as an Integer.
private static final Integer fileLock = new Integer(1);
...
synchronized(fileLock) {
.. do something ..
}
...
It would be much better, in this code, to redeclare fileLock as
private static final Object fileLock = new Object();
The existing code might be OK, but it is confusing and a future refactoring, such as the "Remove Boxing" refactoring in IntelliJ, might replace this with the use of an interned Integer object shared throughout the JVM, leading to very confusing behavior and potential deadlock.
Monitor wait() called on Condition
This method calls wait() on a java.util.concurrent.locks.Condition object.
Waiting for a Condition should be done using one of the await() methods defined by the Condition interface.
A thread was created using the default empty run method
This method creates a thread without specifying a run method either by deriving from the Thread class, or by passing a Runnable object.
This thread, then, does nothing but waste time.
Empty synchronized block
The code contains an empty synchronized block:
synchronized() { }
Empty synchronized blocks are far more subtle and hard to use correctly than most people recognize, and empty synchronized blocks are almost never a better solution than less contrived solutions.
Inconsistent synchronization
The fields of this class appear to be accessed inconsistently with respect to synchronization.
This bug report indicates that the bug pattern detector judged that
- The class contains a mix of locked and unlocked accesses,
- At least one locked access was performed by one of the class's own methods, and
- The number of unsynchronized field accesses (reads and writes) was no more than one third of all accesses, with writes being weighed twice as high as reads
A typical bug matching this bug pattern is forgetting to synchronize one of the methods in a class that is intended to be thread-safe.
You can select the nodes labeled "Unsynchronized access" to show the code locations where the detector believed that a field was accessed without synchronization.
Note that there are various sources of inaccuracy in this detector; for example, the detector cannot statically detect all situations in which a lock is held.
Also, even when the detector is accurate in distinguishing locked vs. unlocked accesses, the code in question may still be correct.
Field not guarded against concurrent access
This field is annotated with net.jcip.annotations.GuardedBy, but can be accessed in a way that seems to violate the annotation.
Synchronization performed on Lock
This method performs synchronization an object that implements java.util.concurrent.locks.Lock.
Such an object is locked/unlocked using acquire() / release() rather than using the synchronized (...) construct.
Synchronization performed on util.concurrent instance
This method performs synchronization an object that is an instance of a class from the java.util.concurrent package (or its subclasses).
Instances of these classes have their own concurrency control mechanisms that are orthogonal to the synchronization provided by the Java keyword synchronized.
For example, synchronizing on an AtomicBoolean will not prevent other threads from modifying the AtomicBoolean.
Such code may be correct, but should be carefully reviewed and documented, and may confuse people who have to maintain the code at a later date.
Using monitor style wait methods on util.concurrent abstraction
This method calls wait(), notify() or notifyAll() on an object that also provides an await(), signal(), signalAll() method (such as util.concurrent Condition objects).
This probably isn't what you want, and even if you do want it, you should consider changing your design, as other developers will find it exceptionally confusing.
Incorrect lazy initialization of static field
This method contains an unsynchronized lazy initialization of a non-volatile static field.
Because the compiler or processor may reorder instructions, threads are not guaranteed to see a completely initialized object, if the method can be called by multiple threads.
You can make the field volatile to correct the problem. For more information, see the Java Memory Model web site.
Incorrect lazy initialization and update of static field
This method contains an unsynchronized lazy initialization of a static field.
After the field is set, the object stored into that location is further updated or accessed.
The setting of the field is visible to other threads as soon as it is set.
If the further accesses in the method that set the field serve to initialize the object, then you have a very serious multithreading bug, unless something else prevents any other thread from accessing the stored object until it is fully initialized.
Even if you feel confident that the method is never called by multiple threads, it might be better to not set the static field until the value you are setting it to is fully populated/initialized.
Synchronization on field in futile attempt to guard that field
This method synchronizes on a field in what appears to be an attempt to guard against simultaneous updates to that field.
But guarding a field gets a lock on the referenced object, not on the field.
This may not provide the mutual exclusion you need, and other threads might be obtaining locks on the referenced objects (for other purposes).
An example of this pattern would be:
private Long myNtfSeqNbrCounter = new Long(0);
private Long getNotificationSequenceNumber() {
Long result = null;
synchronized(myNtfSeqNbrCounter) {
result = new Long(myNtfSeqNbrCounter.longValue() + 1);
myNtfSeqNbrCounter = new Long(result.longValue());
}
return result;
}
Method synchronizes on an updated field
This method synchronizes on an object referenced from a mutable field.
This is unlikely to have useful semantics, since different threads may be synchronizing on different objects.
Mutable servlet field
A web server generally only creates one instance of servlet or jsp class (i.e., treats the class as a Singleton), and will have multiple threads invoke methods on that instance to service multiple simultaneous requests.
Thus, having a mutable instance field generally creates race conditions.
Mismatched notify()
This method calls Object.notify() or Object.notifyAll() without obviously holding a lock on the object.
Calling notify() or notifyAll() without a lock held will result in an IllegalMonitorStateException being thrown.
Mismatched wait()
This method calls Object.wait() without obviously holding a lock on the object.
Calling wait() without a lock held will result in an IllegalMonitorStateException being thrown.
Naked notify
A call to notify() or notifyAll() was made without any (apparent) accompanying modification to mutable object state.
In general, calling a notify method on a monitor is done because some condition another thread is waiting for has become true.
However, for the condition to be meaningful, it must involve a heap object that is visible to both threads.
This bug does not necessarily indicate an error, since the change to mutable object state may have taken place in a method which then called the method containing the notification.
Synchronize and null check on the same field
Since the field is synchronized on, it seems not likely to be null.
If it is null and then synchronized on a NullPointerException will be thrown and the check would be pointless.
Better to synchronize on another field.
Using notify() rather than notifyAll()
This method calls notify() rather than notifyAll().
Java monitors are often used for multiple conditions.
Calling notify() only wakes up one thread, meaning that the thread woken up might not be the one waiting for the condition that the caller just satisfied.
Class's readObject() method is synchronized
This serializable class defines a readObject() which is synchronized.
By definition, an object created by deserialization is only reachable by one thread, and thus there is no need for readObject() to be synchronized.
If the readObject() method itself is causing the object to become visible to another thread, that is an example of very dubious coding style.
Return value of putIfAbsent ignored, value passed to putIfAbsent reused
The putIfAbsent method is typically used to ensure that a single value is associated with a given key (the first value for which put if absent succeeds).
If you ignore the return value and retain a reference to the value passed in, you run the risk of retaining a value that is not the one that is associated with the key in the map.
If it matters which one you use and you use the one that isn't stored in the map, your program will behave incorrectly.
Invokes run on a thread (did you mean to start it instead?)
This method explicitly invokes run() on an object.
In general, classes implement the Runnable interface because they are going to have their run() method invoked in a new thread, in which case Thread.start() is the right method to call.
Constructor invokes Thread.start()
The constructor starts a thread.
This is likely to be wrong if the class is ever extended/subclassed, since the thread will be started before the subclass constructor is started.
Method spins on field
This method spins in a loop which reads a field.
The compiler may legally hoist the read out of the loop, turning the code into an infinite loop.
The class should be changed so it uses proper synchronization (including wait and notify calls).
Call to static Calendar
Even though the JavaDoc does not contain a hint about it, Calendars are inherently unsafe for multihtreaded use.
The detector has found a call to an instance of Calendar that has been obtained via a static field.
This looks suspicous.
Call to static DateFormat
As the JavaDoc states, DateFormats are inherently unsafe for multithreaded use.
The detector has found a call to an instance of DateFormat that has been obtained via a static field.
This looks suspicous.
Static Calendar field
Even though the JavaDoc does not contain a hint about it, Calendars are inherently unsafe for multihtreaded use.
Sharing a single instance across thread boundaries without proper synchronization will result in erratic behavior of the application.
Under 1.4 problems seem to surface less often than under Java 5 where you will probably see random ArrayIndexOutOfBoundsExceptions or IndexOutOfBoundsExceptions in sun.util.calendar.BaseCalendar.getCalendarDateFromFixedDate().
You may also experience serialization problems.
Using an instance field is recommended.
Static DateFormat
As the JavaDoc states, DateFormats are inherently unsafe for multithreaded use.
Sharing a single instance across thread boundaries without proper synchronization will result in erratic behavior of the application.
You may also experience serialization problems.
Using an instance field is recommended.
Method calls Thread.sleep() with a lock held
This method calls Thread.sleep() with a lock held.
This may result in very poor performance and scalability, or a deadlock, since other threads may be waiting to acquire the lock.
It is a much better idea to call wait() on the lock, which releases the lock and allows other threads to run.
Wait with two locks held
Waiting on a monitor while two locks are held may cause deadlock.
Performing a wait only releases the lock on the object being waited on, not any other locks.
This not necessarily a bug, but is worth examining closely.
Unsynchronized get method, synchronized set method
This class contains similarly-named get and set methods where the set method is synchronized and the get method is not.
This may result in incorrect behavior at runtime, as callers of the get method will not necessarily see a consistent state for the object.
The get method should be made synchronized.
Method does not release lock on all paths
This method acquires a JSR-166 (java.util.concurrent) lock, but does not release it on all paths out of the method.
In general, the correct idiom for using a JSR-166 lock is:
Lock l = ...;
l.lock();
try {
// do something
} finally {
l.unlock();
}
Method does not release lock on all exception paths
This method acquires a JSR-166 (java.util.concurrent) lock, but does not release it on all exception paths out of the method.
In general, the correct idiom for using a JSR-166 lock is:
Lock l = ...;
l.lock();
try {
// do something
} finally {
l.unlock();
}
Unconditional wait
This method contains a call to java.lang.Object.wait() which is not guarded by conditional control flow.
The code should verify that condition it intends to wait for is not already satisfied before calling wait; any previous notifications will be ignored.
An increment to a volatile field isn't atomic
This code increments a volatile field.
Increments of volatile fields aren't atomic.
If more than one thread is incrementing the field at the same time, increments could be lost.
A volatile reference to an array doesn't treat the array elements as volatile
This declares a volatile reference to an array, which might not be what you want.
With a volatile reference to an array, reads and writes of the reference to the array are treated as volatile, but the array elements are non-volatile.
To get volatile array elements, you will need to use one of the atomic array classes in java.util.concurrent (provided in Java 5.0).
Synchronization on getClass rather than class literal
This instance method synchronizes on this.getClass().
If this class is subclassed, subclasses will synchronize on the class object for the subclass, which isn't likely what was intended.
For example, consider this code from java.awt.Label:
private static final String base = "label";
private static int nameCounter = 0;
String constructComponentName() {
synchronized (getClass()) {
return base + nameCounter++;
}
}
Subclasses of Label won't synchronize on the same subclass, giving rise to a datarace.
Instead, this code should be synchronizing on Label.class
private static final String base = "label";
private static int nameCounter = 0;
String constructComponentName() {
synchronized (Label.class) {
return base + nameCounter++;
}
}
Class's writeObject() method is synchronized but nothing else is
This class has a writeObject() method which is synchronized; however, no other method of the class is synchronized.
Condition.await() not in loop
This method contains a call to java.util.concurrent.await() (or variants) which is not in a loop.
If the object is used for multiple conditions, the condition the caller intended to wait for might not be the one that actually occurred.
Wait not in loop
This method contains a call to java.lang.Object.wait() which is not in a loop.
If the monitor is used for multiple conditions, the condition the caller intended to wait for might not be the one that actually occurred.
No comments:
Post a Comment
Note: only a member of this blog may post a comment.