0

I'm aware that the incrementing going on in the below code is not atomic. I'd like the incrementing, insertion into blocking queue and printing value of counter all together be an atomic operation. I'm aware of the atomic int but I'm trying to get this to work using synchronization for learning purposes.

int counter = 0;
BlockingQueue<Integer> numbers = new ArrayBlockingQueue<Integer>(100);

while(true) {
    numbers.put(counter++);
    System.out.println("Inserted new number: " + counter);
}
... // other code that may take things off the numbers queue..

What if I define a synchronized method called increment() that increments, prints and returns and pass that to numbers.put(increment());? In that situation, would incrementing, printing and inserting into the blocking queue together be one atomic operation?

user836087
  • 2,271
  • 8
  • 23
  • 33
  • The insertion would be, the increment would not because it's not synchronized. – shmosel Aug 12 '17 at 00:06
  • Now is the time we take a step back and ask, why do you want to insert numbers in a blocking queue in an atomic manner? I mean, other than for esoteric reasons, what is the use case here, if any at all? – Abhijit Sarkar Aug 12 '17 at 00:18
  • Use case = Learning.... – user836087 Aug 12 '17 at 00:18
  • 1
    You got your answer in various forms. `put(counter++)` is not atomic, there are 2 distinct operations involved. To make it atomic, you'll need an external monitor. What else? – Abhijit Sarkar Aug 12 '17 at 00:20

1 Answers1

2

What if I define a synchronized method called increment() that increments, prints and returns and pass that to numbers.put(increment());? In that situation, would incrementing, printing and inserting into the blocking queue together be one atomic operation?

No, you need to synchronize over both operations.

Otherwise the following flow was possible:

  1. Thread A calls increment (gets back 1)
  2. Thread B calls increment (gets back 2, that part works because you synchronized the method it won't get another 1)
  3. Thread B adds to the queue (now the queue has 2 before 1)
  4. Thread A adds to the queue

What happened here is that Thread B "sneaked in" between the two operations for Thread A. The exact same thing can happen even if you use an AtomicInteger to implement the counter. The only way to prevent this invalid interleaved execution is grouping the two operations together behind a lock.

Thilo
  • 257,207
  • 101
  • 511
  • 656
  • Thank you Thilo. That clears things up. But is there an issue if I have a producer consumer type thing where in the producer thread, the incrementing is happening, and in the consumer thread, the removal of the number is happening? In that case, would we run into any race conditions with just 2 threads? One thread, Producer, the other thread consumer? The situation you're mentioning could be problematic only if there are 2 producer threads right? – user836087 Aug 12 '17 at 00:56
  • If you only have one thread that can possibly run through a code path, you do not need to make that code path threadsafe, yes. In your example, if only one thread talks to the counter and adds to the queue, you do not need synchronization. There would only ever be Thread A and no Thread B. – Thilo Aug 12 '17 at 00:59
  • You're a blessing. Thank you. Last question! What if I just use atomic int? Then regardless of the number number of flows going through the process of incrementing, printing and inserting into that queue, it should be thread safe, because of atomic int? – user836087 Aug 12 '17 at 01:03
  • No, you need to synchronize over both operations. With AtomicInteger the above interleaved execution is still possible. AtomicInteger only guarantees that you don't get the same number twice (or missing numbers), just like synchronizing the `increase` method does. – Thilo Aug 12 '17 at 01:05
  • Thank you Thilo. But you mentioned that I'd need to synchronize over both methods. How would I do that? Would I need to do `synchronized(lock) { numbers.put(counter++);` even though it's going into an arrayblockingqueue... Wouldn't this be redundant and too much synchronization? – user836087 Aug 12 '17 at 15:44
  • Yes, that's what you need to do. It is not redundant. Also using an AtomicInteger would be redundant (because the synchronized block already gives you that), but you still need to use the ArrayBlockingQueue (because it also takes care of being thread-safe towards to consumers). – Thilo Aug 12 '17 at 23:49