Programs must not use instance locks to protect static shared data because instance locks are ineffective when two or more instances of the class are created. Consequently, failure to use a static lock object leaves the shared state unprotected against concurrent access. Lock objects for classes that can interact with untrusted code must also be private and final, as shown in LCK00-J. Use private final lock objects to synchronize classes that may interact with untrusted code.
Noncompliant Code Example (Nonstatic Lock Object for Static Data)
This noncompliant code example attempts to guard access to the static counter field using a nonstatic lock object. When two Runnable tasks are started, they create two instances of the lock object and lock on each instance separately.
public final class CountBoxes implements Runnable {
private static volatile int counter;
// ...
private final Object lock = new Object();
@Override public void run() {
synchronized (lock) {
counter++;
// ...
}
}
public static void main(String[] args) {
for (int i = 0; i < 2; i++) {
new Thread(new CountBoxes()).start();
}
}
}
This example fails to prevent either thread from observing an inconsistent value of counter because the increment operation on volatile fields fails to be atomic in the absence of proper synchronization (see VNA02-J. Ensure that compound operations on shared variables are atomic for more information).
Noncompliant Code Example (Method Synchronization for Static Data)
This noncompliant code example uses method synchronization to protect access to a static class counter field:
public final class CountBoxes implements Runnable {
private static volatile int counter;
// ...
public synchronized void run() {
counter++;
// ...
}
// ...
}
In this case, the method synchronization uses the intrinsic lock associated with each instance of the class rather than the intrinsic lock associated with the class itself. Consequently, threads constructed using different Runnable instances may observe inconsistent values of counter.
Compliant Solution (Static Lock Object)
This compliant solution ensures the atomicity of the increment operation by locking on a static object:
public class CountBoxes implements Runnable {
private static int counter;
// ...
private static final Object lock = new Object();
public void run() {
synchronized (lock) {
counter++;
// ...
}
}
// ...
}
It is unnecessary to declare the counter variable volatile when using synchronization.
Risk Assessment
Using an instance lock to protect static shared data can result in nondeterministic behavior.
Rule | Severity | Likelihood | Remediation Cost | Priority | Level |
|---|---|---|---|---|---|
LCK06-J | Medium | Probable | Medium | P8 | L2 |
Automated Detection
Some static analysis tools can detect violations of this rule.
| Tool | Version | Checker | Description |
|---|---|---|---|
| Parasoft Jtest | 2023.1 | CERT.LCK06.INSTLOCK | Do not use an instance lock to protect shared static data |
| SpotBugs | 4.6.0 | SSD_DO_NOT_USE_INSTANCE_LOCK_ON_SHARED_STATIC_DATA | Implemented since 4.6.0 |
Related Guidelines
Bibliography
[API 2014] |
Issue Tracking
100%
Review List
7 Comments
Dhruv Mohindra
There could be a violation without having to declare a variable static, for e.g.:
public void doSomethingSimple(int noOfThreads) { final Complex complex = new Complex(); for (int i = 0; i < noOfThreads; i++) { new Thread(new Runnable() { @Override public synchronized void run() { complex.doSomethingComplex(); } }).start(); } }Ilguiz Latypov
"It is unnecessary to declare the
countervariable volatile when using synchronization". I heard that, unless declared final, references to mutable objects could leak past the end of the synchronized block along with reordered instructions against the referred object's contents. (I understand that the example's counter is not a reference).David Svoboda
I suspect that sentence came from a specific context and doesn't apply here. While reordering is permissible, reordering may not transcend the synchronized block, due to the happens-before guarantee. So while some other class can hold a
CountBoxesobject thru an increment, it cannot 'see' an intermediate version of the object.Ilguiz Latypov
The happens-before guarantee seems to apply to code blocks in threads synchronizing on the same object. (I figured this after reading why Double Checked Locking did not work http://www.javaworld.com/article/2074979/java-concurrency/double-checked-locking--clever--but-broken.html ; page 2 of the article indicates the time of writing before the later Java memory model made "volatile" writes ordered with previous actions in the same thread such as writes to non-volatile members). I agree this LCK06 rule "do not use an instance lock to protect shared static data" implies that every thread's access to shared data is synchronized on the same lock object. I made my original comment under the influence of the DCL fix where the context was shifted towards optimizing the speed of access to the shared data. From that point of view, using "synchronized" would be detrimental and redundant after encapsulating the shared data into another object and declaring the member variable holding the reference to that object "volatile" or "final". (The volatile member would have to be assigned a reference to a new shared data object every time the shared data needs to change as a whole. A single volatile primitive value, even long or double, can be shared safely as JMM requires atomicity of these, even in the absence of "synchronized" in the readers).
public class CountBoxes implements Runnable { private static volatile int counter; private static final Object lock = new Object(); public void run() { while (true) { BoxEvent e = BoxEventQueue.consumeEvent(); if ("exit".equals(e.name)) { return; } if ("newbox".equals(e.name)) { synchronized (lock) { // keep other threads processing subsequent "newbox" events waiting before reading the updated counter counter++; } } if ("countboxes".equals(e.name)) { int snapCounter = counter; // in case we need to use the momentary value more than once here System.out.println(snapCounter); // "volatile" makes counter atomic regardless of its size and without having to use // "synchronized". } } } // ... }or
public class BoxData { private final int counter; private final BoxEvent event; public BoxData(int counter, BoxEvent event) { this.counter = counter; this.event = event; } public int getCounter() { return counter; } public BoxEvent getEvent() { return event; } } public class CountBoxes implements Runnable { private static volatile BoxData boxData = new BoxData(); private static final Object lock = new Object(); public void run() { while (true) { BoxEvent e = BoxEventQueue.consumeEvent(); if ("exit".equals(e.name)) { return; } if ("newbox".equals(e.name)) { synchronized (lock) { // keep other threads processing subsequent "newbox" events waiting before reading the updated counter boxData = new BoxData(boxData.getCounter() + 1, e); } } if ("countboxes".equals(e.name)) { BoxData snapshot = boxData; // to read from the same reference System.out.println("counter: %d, event: %s", snapshot.getCounter(), snapshot.getEvent().name); // "volatile" makes boxData content update happens-before the reference update // even without using "synchronized". } } } // ... }David Svoboda
First, the Goetz article dates from 2001 and Java's memory model has received a major overhaul since then (partially inspired by Goetz himself), so I wouldn't trust the article to apply to modern versions of Java.
I'll refer you to CERT rule LCK10-J. Use a correct form of the double-checked locking idiom for an up-to-date overview on double-checked locking.
As written, the compliant solution does not require volatile because all uses of counter (eg the only use of counter :) are inside a synchronized block, and so they all have happens-before relations...the volatile keyword adds nothing.
Without synchronized blocks volatile can give you some guarantees, but it is not safe to use with counter because the ++ operator is not atomic...it provides a read followed by a write (of the incremented value), but other threads can access or modify the variable between the read and write...see VNA02-J. Ensure that compound operations on shared variables are atomic for details.
Ilguiz Latypov
I agree that synchronization suffices.
Re: non-atomic ++ of a volatile variable, non-synchronized reads will see either the previous or the new value but not a mix of the two.
After a quick search I saw a StackOverflow answer by Hubert Schmid noting that concurrent reads of a volatile variable do not synchronize between each other. So now I expect my code showing the box count going down from time to time, despite the fact that each of the read snapshots is self-consistent. (I guess readers synchronized on the same lock with writers would not be interrupted by the latter, so using "synchronized(lock)" wins where time-wise consistency matters, as long as we tolerate a later of the two read events getting the original data first).
David Svoboda
Your first code example always increments counter. But if name is "countboxes" you might see a value before a concurrent increment or after. The 'volatile' keyword buys you nothing.
Your second code example behaves the same, but you need the 'volatile' qualifier, lest you publish a 'previously-initialized object' and a thread reads the counter int as 0. See TSM03-J. Do not publish partially initialized objects for more information.
It is true that concurrent reads of a volatile variable have no implicit happens-before relationship. But they each have a happens-before relationship with a previous write, so they provide the same value. (This is not true for non-volatile variables...see my discussion with Alexey Shipilev in the comments for LCK10-J.