Java multithreading: unexpected results
I am developing an enterprise application I encountered some problems running applications in a multithreaded environment I am writing a program in which there is a variable whose value is updated (incremented) at a very fast rate (e.g. 10000 updates / persecond) The loop runs a certain iteration, and the value of the variable is incremented and stored in the HashMap Once the loop terminates and the value prints the variable in the HashMap I got the unexpected value of the variable
This is a demo program (please read the comments for better understanding):
class test implements Runnable {
static ConcurrentHashMap<String,Integer> map = new ConcurrentHashMap<>();
static AtomicInteger value_to_be_incremented_stored = new AtomicInteger(0); // variable whose value to be updated
static AtomicInteger i = new AtomicInteger(0); // this runs the loop
@Override
public void run() {
for (i.set(0); i.get() < 100000; i.incrementAndGet()) {
/*
This loop should run 100000 times and when loop terminates according to me value of variable
"value_to_be_incremented_stored" should be 100000 as its value is incremented
100000 times the loop also runs 100000 times.
*/
System.out.println("Thread > " + Thread.currentThread() + " " + value_to_be_incremented_stored.incrementAndGet());
map.put("TC",value_to_be_incremented_stored.intValue());
}
System.out.println("Output by Thread " + Thread.currentThread() + " " + map.toString());
}
public static void main(String[] args) {
test t1 = new test();
Thread thread1 = new Thread(t1);
thread1.setName("Thread 1");
Thread thread2 = new Thread(t1);
thread2.setName("Thread 2");
Thread thread3 = new Thread(t1);
thread3.setName("Thread 3");
Thread thread4 = new Thread(t1);
thread4.setName("Thread 4");
thread1.start();
thread2.start();
thread3.start();
thread4.start();
}
}
Output (variation):
Problem: I'm running a loop 100000 times (i.get() < 100000) and then the variable value_ to_ be_ incremented_ How does the stored value become more than 100000
Solution
I found three flaws There is a race condition in the for loop between the point where the loop counter is compared and the position where the loop counter is increased You should do this in one step to get the atomic operation:
for ( ; i.incrementAndGet() < 100000; ) {
The other is that there is also a competitive condition between your counter increment and the map Even if you increment them in series, any thread can have different values internally (it is located at different points in the loop), and it can put the previous values in the global map Atomicity is required to ensure that the added value is the value you place in the loop
synchronized( lock ) {
value_to_be_incremented_stored.incrementAndGet();
map.put("TC",value_to_be_incremented_stored.intValue());
}
Finally, for some reason, the value generated by < comparison is 99999 I had to use < = to fix it As we discussed in our comments, setting i.set (0) at the beginning of each for loop is not an obvious reason I guess there are four defects.)
class ThreadTestX implements Runnable {
static ConcurrentHashMap<String,Integer> map = new ConcurrentHashMap<>();
static AtomicInteger value_to_be_incremented_stored = new AtomicInteger(0); // variable whose value to be updated
static AtomicInteger i = new AtomicInteger(0); // this runs the loop
static final Object lock = new Object();
@Override
public void run() {
for ( ; i.incrementAndGet() <= 100000; ) {
/*
This loop should run 100000 times and when loop terminates according to me value of variable
"value_to_be_incremented_stored" should be 100000 as its value is incremented
100000 times the loop also runs 100000 times.
*/
synchronized( lock ) {
value_to_be_incremented_stored.incrementAndGet();
// System.out.println("Thread > " + Thread.currentThread() +
// " " + value_to_be_incremented_stored.get());
map.put("TC",value_to_be_incremented_stored.intValue());
}
}
System.out.println("Output by Thread " + Thread.currentThread()
+ " " + map.toString());
}
public static void main(String[] args) {
ThreadTestX t1 = new ThreadTestX();
Thread thread1 = new Thread(t1);
thread1.setName("Thread 1");
Thread thread2 = new Thread(t1);
thread2.setName("Thread 2");
Thread thread3 = new Thread(t1);
thread3.setName("Thread 3");
Thread thread4 = new Thread(t1);
thread4.setName("Thread 4");
thread1.start();
thread2.start();
thread3.start();
thread4.start();
}
}
Output:
run:
Output by Thread Thread[Thread 4,5,main] {TC=100000}
Output by Thread Thread[Thread 3,main] {TC=100000}
Output by Thread Thread[Thread 1,main] {TC=100000}
Output by Thread Thread[Thread 2,main] {TC=100000}
BUILD SUCCESSFUL (total time: 0 seconds)
Ex post judgment: Although the mark is correct, I'm not sure if I'm correct The problem here is that you try to keep three things in sync: the loop counter I, the value to increase, and the map Allowing any of these to be executed outside the synchronization block can invite them to an unexpected state I think the following may be safer:
@Override
public void run() {
for ( ;; ) {
synchronized( lock ) {
if( i.incrementAndGet() <= 100000 ) {
value_to_be_incremented_stored.incrementAndGet();
map.put("TC",value_to_be_incremented_stored.intValue());
}
else
break;
}
}
System.out.println("Output by Thread " + Thread.currentThread()
+ " " + map.toString());
}
This eliminates the need to declare variables as atomicinteger, but I don't see how to ensure that their values don't change when the loop executes (due to some other threads)