Locking On A Reassignable Field
ID |
java.no_synchronize_on_mutable_field |
Severity |
high |
Remediation Complexity |
trivial |
Remediation Risk |
low |
Remediation Effort |
low |
Resource |
Reliability |
Language |
Java |
Tags |
concurrency, reliability |
Description
Reports a synchronized block whose monitor is a field that is neither final nor left alone — the field can be, and somewhere in the same file is, assigned a different object.
Both conditions must hold. A non-final lock field that is never reassigned is not reported: that is a missing keyword, not a concurrency bug.
Rationale
A monitor is an object, not a variable. synchronized (rosterLock) locks whatever instance rosterLock happens to hold at the moment the block is entered — so reassigning the field hands the next thread a different monitor:
class SessionRegistry {
private Object rosterLock = new Object(); // not final
private int active;
void admit() {
synchronized (rosterLock) { // locks the object held right now
active++;
}
}
void resetRoster() {
rosterLock = new Object(); // the next caller locks something else
}
}Once resetRoster runs, a thread entering admit acquires the new object without contest and executes the critical section side by side with a thread still holding the old one. The block compiles, reads as synchronized, and excludes nobody. This is the worst shape a concurrency defect can take, because the code looks correct exactly where it fails.
The rule stays silent on four shapes:
-
synchronized (this)andsynchronized (Outer.this), which have their own rule. The two can never both fire on one block — this one only proceeds when the monitor is a plain field reference. -
Locals and parameters. Each invocation evaluates them afresh, so concurrent callers lock different objects for a different reason.
-
A monitor reached through another object, such as
other.lock— there is no basis to decide who owns that lock. -
Anything the symbol table cannot attribute to a declaration. An unresolved name is not evidence of a defect, and guessing would report ordinary locks on inherited fields.
Remediation
Make the lock field final. The reference can then never change, so every thread contends for the same monitor:
class SessionRegistry {
private final Object rosterLock = new Object();
private int active;
void admit() {
synchronized (rosterLock) {
active++;
}
}
void resetRoster() {
synchronized (rosterLock) { // reset the state, not the lock
active = 0;
}
}
}If the reassignment was the point — swapping the guarded state wholesale — guard the swap with the same fixed lock instead of replacing the lock itself. Where the critical section needs timed or interruptible acquisition, a final java.util.concurrent.locks.ReentrantLock serves the same purpose.