02 Code Locking Don’t Let Lock Issues Become a Worry #
In the previous lecture, I introduced to you the misconception of solving thread safety issues using concurrent containers and other tools. Today, let’s take a look at another important means of solving thread safety problems - locks, and the common mistakes made when using them.
Let me share an interesting case with you first. One day, a classmate said in our group chat, “I think I encountered a JVM bug, it’s so weird.” We were all curious about what bug it was.
So, he posted this piece of code: there is a class with two int-type fields, a and b. There is an add method that incrementally adds both a and b in a loop 10,000 times, and there is another compare method that also loops 10,000 times to check if a is smaller than b. If the condition is met, it prints the values of a and b, and checks if a > b is true.
@Slf4j
public class Interesting {
volatile int a = 1;
volatile int b = 1;
public void add() {
log.info("add start");
for (int i = 0; i < 10000; i++) {
a++;
b++;
}
log.info("add done");
}
public void compare() {
log.info("compare start");
for (int i = 0; i < 10000; i++) {
// is a always equal to b?
if (a < b) {
log.info("a:{},b:{},{}", a, b, a > b);
// should a > b always be false in the end?
}
}
log.info("compare done");
}
}
He created two threads to execute the add and compare methods separately:
Interesting interesting = new Interesting();
new Thread(() -> interesting.add()).start();
new Thread(() -> interesting.compare()).start();
In theory, since both a and b are incremented, they should always be equal, and the first condition in compare should never be met, so no logs should be printed. However, after running the code, not only were there logs printed, but even more oddly, the compare method found that a < b was true:
A classmate in the group saw this problem and laughed, saying, “This is not a JVM bug, it’s clearly a thread safety issue. It’s obvious that you are manipulating two fields, a and b, which introduces thread safety problems. You should lock the add method to ensure that the increment operations on a and b are atomic, so they won’t get mixed up.” Then, he added a lock to the add method:
public synchronized void add()
However, the problem was not solved after adding the lock.
Let’s think carefully about why locks can solve thread safety problems. Because only one thread can acquire the lock, the resource operations in the locked code are thread-safe. However, in this case, only one thread is always executing the add method, so locking only the add method is useless.
The reason for this confusion is that the two threads are executing the business logic in the add and compare methods in an interleaved manner, and these business logics are not atomic: the a++ and b++ operations can be interleaved with the comparison code in the compare method. More importantly, the comparison operation of a involves three steps at the bytecode level: load a, load b, and compare. Although the code is written in one line, it is not atomic.
Therefore, the correct approach is to add method locks to both add and compare methods, ensuring that when the add method is executing, compare cannot read a and b:
public synchronized void add()
public synchronized void compare()
Therefore, before using locks to solve problems, it is essential to clarify what logic we want to protect and what the multi-threaded execution scenario is like.
It is necessary to clarify whether the lock and the protected object are at the same level before locking #
In addition to adding ineffective method locks without analyzing the relationship between threads, business logic, and locks, another common mistake is failure to clarify whether the lock and the object to be protected are at the same level.
We know that static fields belong to classes and can be protected by class-level locks, while non-static fields belong to class instances and can be protected by instance-level locks.
Let’s first look at what’s wrong with this piece of code: a static int field counter and a non-static method wrong are defined in the class Data to implement the accumulation operation of the counter field.
class Data {
@Getter
private static int counter = 0;
public static int reset() {
counter = 0;
return counter;
}
public synchronized void wrong() {
counter++;
}
}
Let’s write some test code:
@GetMapping("wrong")
public int wrong(@RequestParam(value = "count", defaultValue = "1000000") int count) {
Data.reset();
// Multiple threads call the wrong method of different instances of the Data class for a certain number of times
IntStream.rangeClosed(1, count).parallel().forEach(i -> new Data().wrong());
return Data.getCounter();
}
Since it runs 1 million times by default, the output should be 1 million. However, the output on the page is 639242:
Let’s analyze why this problem occurs.
By adding a lock on the non-static method wrong, it can only ensure that multiple threads cannot execute the wrong method of the same instance, but it cannot guarantee that the wrong method of different instances will not be executed. Since the static counter is shared among multiple instances, thread safety issues are bound to occur.
Once we have clarified our thinking, the fix becomes clear as well: define another static field of type Object in the class, and lock this field before operating on the counter.
class Data {
@Getter
private static int counter = 0;
private static Object locker = new Object();
public void right() {
synchronized (locker) {
counter++;
}
}
}
You may ask, why not just define the wrong method as static so that the lock is class-level? It is possible, but we cannot change the code structure just to solve the thread safety issue by converting an instance method to a static method.
If you are interested, you can further explore from the perspective of bytecode and JVM to find out the differences in implementation between synchronized on code blocks and synchronized keyword on methods.
Consider the granularity and scenario of locks when acquiring locks #
Adding the synchronized keyword on methods to implement locking is indeed simple, and as a result, I have seen some business code where almost all methods are synchronized. However, this abusive use of synchronized has the following issues:
-
First, it is unnecessary. In most cases, 60% of business code follows a three-tier architecture, where data flows from a stateless Controller, Service, and Repository to the database. It is unnecessary to use synchronized to protect any data.
-
Second, it can greatly reduce performance. When using the Spring framework, by default, Controller, Service, and Repository are singletons. Adding synchronized will almost restrict the entire program to support only a single thread, resulting in significant performance issues.
Even if we do have some shared resources that need protection, we should try to reduce the granularity of the lock and only lock the necessary code blocks or even the resources themselves that need protection.
For example, in business code, there is an ArrayList that needs protection because it is operated on by multiple threads, and there is also a time-consuming operation (the slow method in the code) that is not related to thread safety. How should we add locks in this case?
The wrong approach is to lock the entire business logic, including both the slow method and the code that operates on the ArrayList, by including them both in a synchronized code block. The better approach is to reduce the granularity of the lock to the minimum and only lock the ArrayList when operating on it.
private List<Integer> data = new ArrayList<>();
// Time-consuming method not related to thread safety
private void slow() {
try {
TimeUnit.MILLISECONDS.sleep(10);
} catch (InterruptedException e) {
}
}
// Incorrect lock implementation
@GetMapping("wrong")
public int wrong() {
long begin = System.currentTimeMillis();
IntStream.rangeClosed(1, 1000).parallel().forEach(i -> {
// Granularity of the lock is too coarse
synchronized (this) {
slow();
data.add(i);
}
});
log.info("took:{}", System.currentTimeMillis() - begin);
return data.size();
}
// Correct lock implementation
@GetMapping("right")
public int right() {
long begin = System.currentTimeMillis();
IntStream.rangeClosed(1, 1000).parallel().forEach(i -> {
slow();
// Lock only the List
synchronized (data) {
data.add(i);
}
});
log.info("took:{}", System.currentTimeMillis() - begin);
return data.size();
}
When executing this code, for the same 1000 business operations, the version with the correct lock implementation takes 1.4 seconds, while locking the entire business logic takes 11 seconds.
If the performance is still not satisfactory after considering the application scope of the lock, we need to consider the granularity from another dimension: distinguishing between read and write scenarios and resource access conflicts, and decide whether to use pessimistic or optimistic locks.
In general business code, it is rare to need to further consider these two more fine-grained locks. So, I will only share a few general conclusions with you, and you can consider whether it is necessary to further optimize based on your own needs:
-
For scenarios where there is a significant difference in the ratio of reads to writes, consider using ReentrantReadWriteLock to distinguish between read and write locks and improve performance.
-
If your JDK version is higher than 1.8 and the probability of conflicts on shared resources is not that high, consider using the optimistic read feature of StampedLock to further improve performance.
-
Both ReentrantLock and ReentrantReadWriteLock in the JDK provide fair lock versions. Unless there is a clear requirement, do not enable the fair lock feature easily. Enabling the fair lock feature in a lightly loaded scenario may cause performance to drop by hundreds of times.
Be Careful with Deadlock Problems when Using multiple Locks #
Earlier, we discussed that the granularity of locks should be sufficient for our needs, which means that sometimes we will have more finely-grained locks in our program logic. However, if a business logic involves multiple locks, it is prone to deadlock problems.
I have encountered a case like this before: the order operation needs to lock the inventory of multiple items in the order. After obtaining all the locks for the items, the order deducts the inventory and then releases all the locks. After the code was deployed, it was found that the order failure rate was very high, and users needed to place the order again, which greatly affected user experience and sales.
After investigation, we found that the root cause was a deadlock caused by the different order of inventory deduction. In a concurrent scenario, multiple threads might hold locks for some items and wait for other threads to release locks for other items, resulting in a deadlock.
Next, let’s analyze the core business code.
First, define an item type that includes the item name, remaining inventory, and item lock. Each item has a default inventory of 1000. Then, initialize 10 objects of this item type to simulate the item list:
@Data
@RequiredArgsConstructor
static class Item {
final String name; // item name
int remaining = 1000; // remaining inventory
@ToString.Exclude // exclude this field from ToString
ReentrantLock lock = new ReentrantLock();
}
Next, write a method to simulate selecting items in a shopping cart. Each time, randomly select three items from the item list (items field). For simplicity, we do not consider the logic of selecting multiple items of the same type each time, so the shopping cart does not reflect the quantity of items:
private List<Item> createCart() {
return IntStream.rangeClosed(1, 3)
.mapToObj(i -> "item" + ThreadLocalRandom.current().nextInt(items.size()))
.map(name -> items.get(name)).collect(Collectors.toList());
}
The order code is as follows: first declare a List to store all obtained locks, then iterate through the items in the shopping cart and try to obtain the lock for each item. If the lock is not obtained within 10 seconds, release all previously obtained locks and return false to indicate an order failure.
private boolean createOrder(List<Item> order) {
// store all obtained locks
List<ReentrantLock> locks = new ArrayList<>();
for (Item item : order) {
try {
// try to obtain the lock with a timeout of 10 seconds
if (item.lock.tryLock(10, TimeUnit.SECONDS)) {
locks.add(item.lock);
} else {
locks.forEach(ReentrantLock::unlock);
return false;
}
} catch (InterruptedException e) {
}
}
// deduct inventory logic after obtaining all locks
try {
order.forEach(item -> item.remaining--);
} finally {
locks.forEach(ReentrantLock::unlock);
}
return true;
}
Let’s write a code snippet to test this order operation. Simulate 100 cart creation and order operations in a multi-threaded scenario, and finally output the number of successful orders, the total remaining number of items, the time it took for 100 orders, and the inventory details after the orders are completed:
@GetMapping("wrong")
public long wrong() {
long begin = System.currentTimeMillis();
// perform 100 order operations concurrently and count the number of successful orders
long success = IntStream.rangeClosed(1, 100).parallel()
.mapToObj(i -> {
List<Item> cart = createCart();
return createOrder(cart);
})
.filter(result -> result)
.count();
log.info("success:{} totalRemaining:{} took:{}ms items:{}",
success,
items.entrySet().stream().map(item -> item.getValue().remaining).reduce(0, Integer::sum),
System.currentTimeMillis() - begin, items);
return success;
}
When running the program, the following log is output:
As can be seen, the order operation was successful 65 times out of 100 attempts. There are a total of 10 items with a total inventory count of 10000 and a remaining inventory count of 9805. This meets the expectation (65 successful orders, with each order containing three items), and it took a total of 50 seconds.
Why did this happen?
Let’s use the built-in VisualVM tool in JDK to track it. After executing the method again, we can see in the Threads tab that there is a deadlock issue. Click the “Dump” button on the right side to capture the stack trace:
Look at the captured thread stack trace. In the middle of the page, we can see the following log:
Obviously, a deadlock has occurred. Thread 4 is waiting for a lock held by Thread 3, and Thread 3 is waiting for another lock held by Thread 4.
So why did the deadlock happen?
Let’s recall the logic of adding items to the shopping cart. If we assume that one shopping cart contains item1 and item2, and another shopping cart contains item2 and item1, if one thread obtains the lock for item1 and another thread obtains the lock for item2, then both threads need to obtain the locks for item2 and item1 respectively. At this point, the locks have been acquired by the other thread, so they can only wait for each other until the timeout of 10 seconds.
In fact, avoiding deadlock is quite simple: sort the items in the shopping cart so that all threads obtain the lock for item1 before obtaining the lock for item2, and there will be no problem. So I only need to modify one line of code to sort the shopping cart obtained by createCart by item name:
@GetMapping("right")
public long right() {
....
long success = IntStream.rangeClosed(1, 100).parallel()
.mapToObj(i -> {
List<Item> cart = createCart().stream()
.sorted(Comparator.comparing(Item::getName))
.collect(Collectors.toList());
return createOrder(cart);
})
.filter(result -> result)
.count();
...
return success;
}
Test the right method. No matter how many times it is executed, it always succeeds in ordering 100 times, and the performance is quite high, reaching over 3000 TPS:
In this case, although a deadlock problem occurred, it did not cause permanent deadlock because the attempt to acquire the lock does not block indefinitely. The improvement is to avoid circular waiting by sorting the items in the shopping cart and acquiring locks in a specific order, thus avoiding circular waiting.
Key Recap #
Let’s summarize the pitfalls of using locks to solve thread safety problems in multi-threaded scenarios.
Firstly, although using synchronized to lock is simple, we need to first determine whether the shared resource is at the class level or instance level, and which threads will operate on it. We also need to understand the scope of the lock object or method associated with synchronized.
Secondly, when locking, we should consider granularity and scenarios. Locking the code implies that multi-threaded operations cannot be performed. For web applications with inherent multi-threading, locking methods extensively will significantly degrade concurrency. Therefore, it is advisable to only lock the necessary code blocks and reduce the granularity of locks. For businesses that require high performance, it is important to consider lock read and write scenarios, pessimistic vs optimistic locking, and fine-tuned locking schemes for specific scenarios. Advanced lock utility classes like ReentrantReadWriteLock and StampedLock can be used in appropriate scenarios.
Thirdly, when multiple locks are present in the business logic, we need to consider the possibility of deadlocks. Common avoidance strategies include avoiding infinite waiting and circular waiting.
In addition, if the implementation of locks in the business logic is complex, we need to carefully check whether locking and releasing are paired, and whether there is a possibility of missing or repeating releases. For distributed locks, automatic timeout release should be considered. If the business logic is still in progress, but another thread or process acquires the same lock, it may lead to duplicate execution.
For the sake of demonstration, today’s case involves creating new threads or using thread pools for concurrent simulations in the logic of a controller. We can certainly be aware of the objects that are undergoing concurrent operations. However, for web applications with inherent multi-threading scenarios, you may be more likely to overlook this point and potentially reduce the overall throughput of the application.
The code used today has been uploaded to GitHub. You can click this link to view it.
Reflection and Discussion #
-
In the example at the beginning of this article, the variables
aandbare both declared with thevolatilekeyword. Do you know why? I encountered a pitfall before: we started a thread with an infinite loop to run some tasks, and there is a bool variable used to control the loop exit, with the default value oftrueindicating execution. After a period of time, the main thread sets this variable tofalse. If this variable is not declared asvolatile, can the child thread exit? Can you explain the reason behind this? -
At the end of the article, we mentioned two more pitfalls: the issue of mismatched locking and unlocking, and the problem of repeated logic execution caused by automatic lock release. Do you have any methods to discover and solve these two problems?
Have you encountered any other pitfalls while using locks? I am Zhu Ye, and I welcome you to leave your thoughts in the comments section and share this article with your friends or colleagues to exchange ideas.