There are two concurrency issues with your code.
The shared int i is neither appropriately declared nor accessed for concurrent tasks. It need to be at least volatile but in order to correctly handle an atomic increment should be an AtomicInteger. Volatile would help guarantee that other threads can see if the value changes (which is not otherwise guaranteed by the JVM), but atomic integer goes one further and provides atomic operations which is what you really need here.
Your loop can be accessed by both threads simultaneously since the condition check is separate from the increment of the variable. To correctly handle concurrent threads running your loop you need the while loop condition here to also do the increment as a single atomic operation. Either that or you need to add a synchronized block.
Maybe try something like the below untested code:
public class Counter {
private final AtomicInteger i = new AtomicInteger(1);
public void add() {
for (;;)
{
int localCount = i.getAndIncrement();
if (localCount > 5) { break; }
System.out.println(localCount + " " + Thread.currentThread().getName());
}
}
}
To explain the above code line by line:
- We switched
i to an AtomicInteger since it allows us to get the current value and atomically increment the next value by one. This atomicness is important since otherwise two competing threads could increment to the same value instead of reading and incrementing to each value exactly once across all threads. We could achieve the same by putting the incrementing portion of the code in a synchronized block.
- We switched to a
for (;;) loop so that we can move the loop control / termination code to within the loop body. This allows us to easily use the result of the atomic increment both for control code and for usage within the body of the loop (such as in the println statement).
i.getAndIncrement() atomically increments our counter and returns to us the value before incrementing. This operation is guaranteed to work across however many threads are operating and ensures we will not miss or duplicate any numbers. Assigning the previous count value to a localCount int provides us the value to use within our loop. If we were to try to re-access i within our loop instead of using localCount, we would end up with a different number at some point since other threads are concurrently incrementing i.- We check
localCount to see if it has exceed our termination limit, and if it has we break our loop. We don't check i since it may have been modified already by another thread, and we are only interested in whether our current number that this thread has exceeds the limit or not.
- Lastly we do our println using
localCount instead of i, since i has probably been modified by another thread by the time the code gets here (which would result in printing duplicate lines).
One note, this code does not guarantee that the numbers of the counter will be printed in order. It is likely that most of the time that could be the case, but it is certainly not guaranteed and should not be depended on. Your question did not state the actual expected / required results, and if this is a requirement then you most likely need to synchronize around both the increment and println statements to ensure no out of order execution.
e.g. synchronized solution:
public class Counter {
int i = 1;
public void add() {
for (;;) {
synchronized(this) {
if (i > 5) { break; }
System.out.println(i + " " + Thread.currentThread().getName());
i++;
}
}
}
}
