This can be fixed a number of ways.
Option 1: Method level synchronization. This is the simplest. As you can see, the increment() method is synchronized, so that the other threads must wait for the thread that already has the lock to execute that method.
import java.util.HashMap;
import java.util.Map;
public class Counter {
//shared variable or resource
private Integer count = Integer.valueOf(0);
private Map<String, Integer> userToNumber = new HashMap<String, Integer>(10);
public synchronized void increment() {
try {
count = count + 1;
Thread.sleep(50);
Thread thread = Thread.currentThread();
userToNumber.put(thread.getName(), count);
} catch (InterruptedException e) {
e.printStackTrace();
}
}
public Integer getCount(String name) {
return userToNumber.get(name);
}
}
Option 2: Even though the Option 1 is simple, it locks the entire method and can adversely impact performance for long running methods as each thread has to execute the entire method one at a time. So, the Option 1 can be improved by providing block level lock. Lock only those operations that are acting on the shared resource and making it non-atomic.
The code below uses an Object, which has its own lock to ensure that two threads cannot execute both the Operation 1 and 2 at the same time because there is only one lock.
import java.util.HashMap;
import java.util.Map;
public class Counter {
//shared variable or resource
private Integer count = Integer.valueOf(0);
private Map<String, Integer> userToNumber = new HashMap<String, Integer>(10);
private Object mutex = new Object(); // a lock
public void increment() {
try {
synchronized(mutex) {
count = count + 1; //operation 1
Thread.sleep(50);
Thread thread = Thread.currentThread();
userToNumber.put(thread.getName(), count); //operation 2
}
// there could be other operations here that uses the shared resource as read only
} catch (InterruptedException e) {
e.printStackTrace();
}
}
public Integer getCount(String name) {
return userToNumber.get(name);
}
}
Option 3: This is a very trivial, but practical example. The Java 5 introduced locks and locks are better than using just objects for more flexible locking scenarios where Locks can be used in place of synchronized blocks. Locks offer more flexibility than synchronized blocks in that a thread can unlock multiple locks it holds in a different order than the locks were obtained. Here is the code that replaces synchronized with a reentrant lock. Synchronized blocks in Java are reentrant, which means if a Java thread enters a synchronized block of code, and thereby take the lock on the object the block is synchronized on, the thread can enter other Java code blocks synchronized on the same lock object.
For example, here is the demo of reentrant lock.
public class Reentrant{
public synchronized method1(){
method2(); //calls another synchronized method on the same object
}
public synchronized method2(){
//do something
}
}
Here is the Option 3 example using a ReentrantLock.
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
public class Counter {
// shared variable or resource
private Integer count = Integer.valueOf(0);
private Map<String, Integer> userToNumber = new HashMap<String, Integer>(10);
private Lock mutex = new ReentrantLock(); // a lock
public void increment() {
try {
mutex.lock();
try {
count = count + 1;
Thread.sleep(50);
Thread thread = Thread.currentThread();
userToNumber.put(thread.getName(), count);
} finally {
mutex.unlock(); // finally block is executed even if an
// exception is thrown
}
} catch (InterruptedException e) {
e.printStackTrace();
} finally {
}
}
public Integer getCount(String name) {
return userToNumber.get(name);
}
}
Note that the locks are unlocked in a finally block as it is executed even if an exception is thrown.
The output for the above 3 options will be something like shown below. The order cannot be guaranteed.
But you will get unique numbers assigned for each user.
User-1 value is 1
User-3 value is 2
User-2 value is 3