Volatile, DCL, and synchronization pitfalls in Java
Tags: java, programming, coding
Published: 2024-06-05 11:10:28

Author:Konstantin Volohovsky

What if common knowledge is actually more nuanced, and old familiar things like Double\-checked locking are quite controversial? Examining the code of real projects gives this kind of thought\. In this article, we'll discuss the results of the examination\.

![](https://import.viva64.com/docx/blog/1128_parallel_pitfalls/image1.png)

## Introduction

Not so long ago, as part of my work routine related to checking open\-source projects using PVS\-Studio, I checked the newly released 24th version of the well\-known [DBeaver](https://github.com/dbeaver/dbeaver) project\. I was pleasantly surprised by the quality of its code — the fact that developers use static analysis tools doesn't go to waste\. However, I kept digging and found some suspicious code fragments that caught my eye\. They were so conspicuous that I've decided to dedicate an article to each of them\. So, welcome to the first part of the series\.

## Thread\-unsafe programming

### What am I talking about?

Parallel programming has many pitfalls\. Of course, not only in Java, as its evil \(or good :\) \) twin brother C\# can easily make you stumble when using [events](https://pvs-studio.com/en/docs/warnings/v3083/) in parallel scenarios\. Asynchronous and multithreaded errors are often the most difficult to catch\.

Below, we'll look at three types of such errors\. The first one is the non\-atomic change of volatile variables\.

### What's volatile again?

The [documentation](https://docs.oracle.com/javase/tutorial/essential/concurrency/atomic.html) tells what the *volatile* keyword does:

1. It prevents each cache from being used by different threads\. This ensures that changes to a variable are immediately visible to all threads\.
1. It sets a happens\-before relationship between a variable and read/write, thus prohibiting optimizations based on changing the order of operations\.

You can see how this keyword works with a simple example of a thread waiting for a variable to change:

```cpp
private static volatile String str;
public static void main() throws InterruptedException {
    new Thread(() -> {
        while (str == null) { }
        System.out.println(str);
    }).start();

    Thread.sleep(1);

    str = "Hello World!";
}
```

If we remove *volatile*, the loop becomes infinite\. At least in my case\. The result can be highly dependent on the JVM and hardware\.

<spoiler title="How can we see happens-before in action, though?">

If we change the example a bit by adding a bound variable, the happens\-before mechanism becomes clearer:

```cpp
private static String str;
private static volatile boolean flag = false;
public static void main() throws InterruptedException {
    new Thread(() -> {
        while (!flag) { }
        System.out.println(str);
    }).start();

    Thread.sleep(1);

    str = "Hello World!";
    flag = true;
}
```

If we had **only** a visibility guarantee, the compiler or processor could reverse the order of assigning operations to the *str* and *flag* variables\. Then we'd have a chance to get into *System\.out\.println\(str\)* when *str* is still *null*\. 

However, the happens\-before principle ensures that the write that precedes the write in the volatile variable remains where it's supposed to be\.

</spoiler>


### Race

Even though we have variable visibility, it's certainly not enough to get parallel threads to operate properly:

```cpp
private static volatile int counter = 0;
private static void increment() {
    Thread.sleep(1);
    counter++;
}
public static void main() {
    var threads = Stream.generate(() -> new Thread(Main::increment))
                        .limit(100)
                        .toList();

    threads.forEach(Thread::start);
    while (threads.stream().anyMatch(Thread::isAlive)) {
        Thread.yield();
    }

    System.out.println(counter);
}
```

Such code always returns a number less than 100\. This isn't surprising, since the increment operation isn't atomic, and one thread can squeeze through the other\. This can happen between the operations of taking the old *counter* value and incrementing it by 1\. There can be two fixes, one easier than the other:

<spoiler title="Atomic types">

Let's use special data types that provide a ready\-made interface for working from multiple threads\. In this case, it's *AtomicInteger*\.

```cpp
private static final AtomicInteger counter = new AtomicInteger(0);
private static void increment() {
    Thread.sleep(1);
    counter.incrementAndGet();
}
```

</spoiler>




<spoiler title="Synchronization">

Let's just add the *synchronized* keyword \(or the *synchronized* block\)\. This ensures that only one thread can perform operations in this method\.

```cpp
private synchronized static void increment() {
    Thread.sleep(1);
    counter++;
}
```

</spoiler>


### Case study

If you thought that this is basic knowledge and nobody would make the above mistake in real projects, then\.\.\. you're almost right\. I found three fragments in DBeaver where the *volatile* variables of primitive types are handled\. And, as it often happens in real life, the cases aren't so simple\. A dangerous situation may occur in some of them, but for the most part nothing terrible can happen\. Nevertheless, here are these three fragments:

<spoiler title="Counting active operations">

```cpp
public class MultiPageWizardDialog extends .... {
    ....
    private volatile int runningOperations = 0;

    ....
 
    @Override
    public void run(....) {
        ....
        try {
            runningOperations++;               // <=
            ModalContext.run(
               runnable,
               true,
               monitorPart,
               getShell().getDisplay()
            );
        } finally {
            runningOperations--;               // <=
            ....
        }
    }
}
```

The PVS\-Studio warnings:

* [V6074](https://pvs-studio.com/en/docs/warnings/v6074/) Non\-atomic modification of volatile variable\. Inspect 'runningOperations'\. MultiPageWizardDialog\.java\(590\)
* [V6074](https://pvs-studio.com/en/docs/warnings/v6074/) Non\-atomic modification of volatile variable\. Inspect 'runningOperations'\. MultiPageWizardDialog\.java\(593\)



We have two warnings for the price of one, and it's also the only fragment of code where there's a tangible risk\. It would seem that we have a reference case that is almost identical to the artificially created example above\. However, if we take a closer look at the name of the class and the method where everything happens \(*MultiPageWizardDialog* and *run*, respectively\), it becomes clear that we're dealing with UI\. It's very unlikely, if at all possible, that a real race condition can occur here\.

However, if we imagine that the *run* method would be executed simultaneously by multiple threads, the value of *runningOperations* could easily be incorrect\.

</spoiler>




<spoiler title="Progress drawing">

```cpp
private volatile int drawCount = 0;

....

private void showProgress() {
    if (loadStartTime == 0) {
        return;
    }
    if (progressOverlay == null) {
        ....
        painListener = e -> {
            ....
            Image image = DBeaverIcons.getImage(
                PROGRESS_IMAGES[drawCount % PROGRESS_IMAGES.length]
            );
            ....
        };
        ....
    }
    drawCount++;
    ....
}
```

The PVS\-Studio warning: 

[V6074](https://pvs-studio.com/en/docs/warnings/v6074/) Non\-atomic modification of volatile variable\. Inspect 'drawCount'\. ProgressLoaderVisualizer\.java\(192\)



Looks like the least dangerous code snippet: *drawCount* is used as a progress image selector, providing a cyclic scrolling index\. Firstly, it seems that the cost of making a mistake in such a code fragment isn't the highest\. Secondly, the very fact of accessing drawing from multiple **parallel** threads sounds questionable\. If the latter assumption is correct, then the *volatile* modifier is redundant here\.

</spoiler>




<spoiler title="Tracking number of initializations">

The analyzer issues a warning for the following code:

```cpp
private volatile int initializedCount = 0;

....
public CompareObjectsExecutor(CompareObjectsSettings settings)
{
    ....
    initializeFinisher = new DBRProgressListener() {
        @Override
        public void onTaskFinished(IStatus status)
        {
            if (!status.isOK()) {
                initializeError = status;
            } else {
                initializedCount++;
            }
        }
    };
    ....
}
```

The PVS\-Studio warning: 

[V6074](https://pvs-studio.com/en/docs/warnings/v6074/) Non\-atomic modification of volatile variable\. Inspect 'initializedCount'\. CompareObjectsExecutor\.java\(130\)



This fragment looks more dangerous then the others\. For now, however, we lack any context, so we need to look at where *initializeFinisher* and *initializecCount* are used:

```cpp
this.initializedCount = 0;

for (DBNDatabaseNode node : nodes) {
    node.initializeNode(null, initializeFinisher);
    ....
}
while (initializedCount != nodes.size()) {
    Thread.sleep(50);
    if (monitor.isCanceled()) {
        throw new InterruptedException();
    }
    ....
}
```



This is what we have:

1. the counter is reset;
1. its increment is triggered in *node\.initializeNode*;
1. next, all nodes are checked for being initialized\. Otherwise, the thread sleep occurs for 50 milliseconds\.

It would seem to be a reference case: if two threads increment a counter at the same time, all logic breaks\. Right? No, there are no threads in *node\.initializeNode*\. There's nothing at all in there :\)

```cpp
public boolean initializeNode(
    DBRProgressMonitor monitor,
    DBRProgressListener onFinish
) throws DBException {
    if (onFinish != null) {
        onFinish.onTaskFinished(Status.OK_STATUS);
    }
    return true;
}
```

Anyway, while *DBRProgressListener* is indeed used in parallel scenarios, this isn't the case\. For *initializedCount*, *volatile* just seems redundant, as does the loop that checks the number of initializations\.

</spoiler>


### Unexpected twist

Earlier, we looked at simple examples with primitive types\. Is it enough to mark the reference type field as volatile to ensure similar behavior? Nope\. You can learn more [here](https://wiki.sei.cmu.edu/confluence/display/java/CON50-J.+Do+not+assume+that+declaring+a+reference+volatile+guarantees+safe+publication+of+the+members+of+the+referenced+object)\. By modifying the previous example a bit, we can easily get an infinite loop again:

```cpp
private static class Proxy {
    public String str;
}
private static volatile Proxy proxy = new Proxy();
public static void main() throws InterruptedException {
    var proxy = Main.proxy;

    new Thread(() -> {
        while (proxy.str == null) { }
        System.out.println(proxy.str);
    }).start();

    Thread.sleep(1);

    proxy.str = "Hello World!";
}
```

One possible fix is to mark the *str* field as volatile, and then everything will work as before\. As for the *proxy* field, there's no point in marking it in any way\.

<spoiler title="Here's the fixed code:">

```cpp
private static class Proxy {
    public volatile String str;
}
private static Proxy proxy = new Proxy();
public static void main() throws InterruptedException {
    var proxy = TaskRunner.proxy;

    new Thread(() -> {
        while (proxy.str == null) { }
        System.out.println(proxy.str);
    }).start();

    Thread.sleep(1);

    proxy.str = "Hello World!";
}
```

</spoiler>




So, is there a point in marking the reference field as volatile? This is where we finally come to the Double\-checked locking pattern\.

## Double\-checked locking

### Why?

This [pattern](https://java-design-patterns.com/patterns/double-checked-locking/) is used as a high\-performance way to implement lazy initialization in a multi\-thread environment\. The idea is quite simple: a normal check is performed before the "heavy" check in the synchronized block\. The control flow continues only if the requested resource hasn't been created yet\. The implementation seems really simple:

```cpp
public class HolderThreadSafe {
    private volatile ResourceClass resource;

    public ResourceClass getResource() {
        if (resource == null) {
            synchronized (this) {
                if (resource == null) {
                    resource = new ResourceClass();
                }
            }
        }
        return resource;
    }
}
```

The pattern is most often used to create singletons and for delayed initialization of heavy objects\. Is it really that simple, though?

### Is it an anti\-pattern?

This pattern has a rather amusing feature\. [It doesn't work](https://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html)\. At least if you're about the same age as dinosaurs, and you're using a Java version lower than 5\. Stay tuned, though\! In fact, this is a key point in our story :\)

So, what has changed in JDK 5? This is when developers added the *volatile* keyword\. Now, you can immediately catch me out and say that in the example above, the *resource* is marked as volatile\. However, if I were you, I'd take a step back and look at the code\. What exactly is the issue here? Here's the order of actions:

1. *resource* is checked for *null*\. This point can be traversed by multiple threads simultaneously;
1. The synchronized block is accessed\. Only one thread can get in there:
    * in the second check, the first thread that did this makes sure that *resource* isn't created yet and then creates it;
1. The other threads that passed the first check access the synchronized block one at a time:
    * they see that *resource* is already created, and they go out of there;
1. Everybody's satisfied and happy\.

Why do we need *volatile* if we use synchronization to control the access to the field? The thread value visibility isn't an obstacle here, really, but we have two issues:

* the operation of creating an object **isn't atomic** \(there are no issues with primitive types, except for the 64\-bit ones\);
* by default, both the compiler and lower\-level systems are allowed to **override the operation order**\.

This means that the consequences of such optimizations can be unpredictable\. For example, the compiler can inline the constructor and change the order of field initialization in any way possible\. As a result, an object may be marked as created\. Another thread sees this, starts using it, and finds uninitialized fields there\. The happens\-before principle described above saves us from this\. Thanks to the volatile variables, double\-check locking works since the fifth version of Java\.

### So, is it a pattern after all?

What made me delve so deeply into the theory? It was another analyzer warning that has been issued for the next method of the DBeaver project:

```cpp
private List<DBTTaskRun> runs;
....
private void loadRunsIfNeeded() {
    if (runs == null) {
        synchronized (this) {
            if (runs == null) {
                runs = new ArrayList<>(loadRunStatistics());
            }
        }
    }
}
```

[V6082](https://pvs-studio.com/en/docs/warnings/v6082/) Unsafe double\-checked locking\. The field should be declared as volatile\. TaskImpl\.java\(59\), TaskImpl\.java\(317\)

It's funny that the [pattern collection](https://java-design-patterns.com/patterns/double-checked-locking/#applicability) I mentioned earlier lists the following drawback:

> Complex implementation can lead to mistakes, such as incorrect publishing of objects due to memory visibility issues\.

Obviously, this is exactly what happened in this case :\) 

Let's return to the question I asked in the theoretical part of the article:

> So, is there a point in marking the reference field as volatile?

Back then, I cheated and deliberately omitted the obvious thing\. When we change an object field, we don't touch the variable containing the object reference in any way\. However, when the variable is **overwritten**, all information specified in that section becomes relevant again\. So, even with the aforementioned limitations \(a volatile field doesn't guarantee the safe publication for the members of the object it references\), there are use cases, and this is one of them\.

### Or is it, though?\!

It seems that the fix in this case is obvious: just mark the *runs* list as the volatile variable\. This resolves the issue\. However, I suggest that you remember the purpose of the pattern:

> The Double\-Checked Locking pattern aims to reduce the overhead of acquiring a lock by first testing the locking criterion \(the 'lock hint'\) without actually acquiring the lock\. Only if the locking criterion check indicates that locking is necessary does the actual locking logic proceed\.

So, we have a pattern that's designed for optimization\. What could be the issue here? Here's a hint: do you remember my comment about dinosaurs? :\)

This pattern was born a **long** time ago, when *synchronized* methods were very slow\. Today, both JVM and hardware performance have advanced significantly\. So, the risk of such errors makes it expensive to use this \(micro\)optimization\. If we talk only about singletons, initializing them as a class field is sufficient for the thread safety:

```cpp
class Holder {
    static SomeSingleton singleton = new SomeSingleton();
}
```

In other cases, we can just use synchronized methods, unless everything else is already optimized\.

By the way, since I've mentioned C\# before, I'll do it again\. Funny enough, the exact same issue exists there too, as we've [written about before](https://pvs-studio.com/en/blog/posts/csharp/0715/)\.

### Another unexpected twist

Should we use synchronized methods? What if the developers are already doing this? In that case, the *volatile* field would simply be redundant\. It's used in 20 code fragments, let's look at a random three:

<spoiler title="One">

```cpp
void addNewRun(@NotNull DBTTaskRun taskRun) {
    synchronized (this) {
        loadRunsIfNeeded();

        runs.add(taskRun);

        while (runs.size() > MAX_RUNS_IN_STATS) {
            runs.remove(0);
        }

        flushRunStatistics(runs);
    }

    TaskRegistry.getInstance().notifyTaskListeners(....);
}
```



Okay, we immediately found a fragment where it's used four times, but there's also a *synchronized* block here\.\.\.

</spoiler>




<spoiler title="Two">

```cpp
private void loadRunsIfNeeded() {
    if (runs == null) {
        synchronized (this) {
            if (runs == null) {
                runs = new ArrayList<>(loadRunStatistics());
            }
        }
    }
}
```

Here we have three more for the price of one and *synchronized* again\. Have I really just been messing with your heads all this time?

</spoiler>




<spoiler title="Three">

```cpp
@Override
public void cleanRunStatistics() {
    Path statsFolder = getTaskStatsFolder(false);
    if (Files.exists(statsFolder)) {
        ....
    }
    if (runs != null) {
        runs.clear();
    }
    flushRunStatistics(List.of());
    TaskRegistry.getInstance().notifyTaskListeners(....);
}
```

Gotcha\! If we follow the uses of this method, there's no synchronization anywhere above\. So, we've found what we've been looking for\. By the way, this fragment isn't the only one\.

</spoiler>


What was the need for this performance if I could just show you a snippet without synchronization? This is just another [potential vulnerability](https://pvs-studio.com/en/docs/warnings/v6102/)\. Its presence alone can cause a race condition in addition to making possible the consequences of a double\-check locking error\.

So, it'll take a little more effort to fix that code fragment, because while we were analyzing one bug, another bug was found\.

## Conclusion

I hope you enjoyed diving into the wonderful world of parallel programming pitfalls with me\. Many people claim that if you've ever had to use volatile variables or the double\-checked locking pattern, you've already done something wrong\. Or you're an expert and you knew exactly what you were doing :\)

Still, I think this note sums up the content of the whole article pretty well: the rabbit hole is deep, indeed\. At least it's deep enough to be twice as careful when dealing with the topic of the article\.

If you'd like to search for those or other errors in your project, you may try PVS\-Studio for free by following [this link](https://pvs-studio.com/en/pvs-studio/try-free/?utm_source=website&utm_medium=habr&utm_campaign=article&utm_content=1128)\.

I'd also like to remind you that these aren't all the interesting things you can find in [DBeaver](https://github.com/dbeaver/dbeaver), — more articles are coming soon\. I'll add links here as soon as they're released\. In the meantime, I'd recommend subscribing to my [X\-twitter](https://x.com/kvolokhovskii), so you don't miss their release\.