How do i prevent my consumer-threads from removing the last element twice?

Question

Questions:

1. Why do I get a NoSuchElementException when trying to remove the last element?
2. How can I fix that?

I have 3 classes (see below) that add/remove Integers to a LinkedList. Everything works fine until the removing Threads get to the last element.

It seems like both threads try to remove it. The first one succeeds, the second one can´t. But I thought the synchronized-method/synchroniced-attribute + !sharedList.isEmpty() would handle that.

Class Producer: This class is supposed to created random numbers, put them in the sharedList, write to console that it just added a number and stop once it gets interrupted. Only 1 thread of this class is expected.

import java.util.LinkedList;

    public class Producer extends Thread
    {

        private LinkedList sharedList;
        private String name;

        public Producer(String name, LinkedList sharedList)
        {
            this.name = name;
            this.sharedList = sharedList;
        }

        public void run()
        {
            while(!this.isInterrupted())
            {
                while(sharedList.size() < 100)
                {
                    if(this.isInterrupted())
                    {
                        break;
                    } else 
                    {
                        addElementToList();
                    }
                }
            }
        }

        private synchronized void addElementToList() 
        {
            synchronized(sharedList)
            {
                sharedList.add((int)(Math.random()*100));
                System.out.println("Thread " + this.name + ": " + sharedList.getLast() + " added");
            }
            try {
                sleep(300);
            } catch (InterruptedException e) {
                this.interrupt();
            }
        }
    }

Class Consumer: This class is supposed to remove the first element in the sharedList, if it exists. The execution should continue (after being interrupted) until sharedList is empty. Multiple (atleast 2) threads of this class are expected.

import java.util.LinkedList;

public class Consumer extends Thread
{
    private String name;
    private LinkedList sharedList;

    public Consumer(String name, LinkedList sharedList)
    {
        this.name = name;
        this.sharedList = sharedList;
    }

    public void run()
    {
        while(!this.isInterrupted())
        {
            while(!sharedList.isEmpty())
            {
                removeListElement();
            }
        }
    }

    private synchronized void removeListElement()
    {
        synchronized(sharedList)
        {
            int removedItem = (Integer) (sharedList.element());
            sharedList.remove();
            System.out.println("Thread " + this.name + ": " + removedItem + " removed");
        }
        try {
            sleep(1000);
        } catch (InterruptedException e) {
            this.interrupt();
        }
    }
}

Class MainMethod: This class is supposed to start and interrupt the threads.

import java.util.LinkedList;


public class MainMethod 
{

    public static void main(String[] args) throws InterruptedException 
    {
        LinkedList sharedList = new LinkedList();
        Producer producer = new Producer("producer", sharedList);
        producer.start();
        Thread.sleep(1000);
        Consumer consumer1 = new Consumer("consumer1", sharedList);
        Consumer consumer2 = new Consumer("consumer2", sharedList);
        consumer1.start();
        consumer2.start();
        Thread.sleep(10000);
        producer.interrupt();
        consumer1.interrupt();
        consumer2.interrupt();
    }

}

Exception: This is the exact exception I get.

Exception in thread "Thread-2" java.util.NoSuchElementException at java.util.LinkedList.getFirst(LinkedList.java:126) at java.util.LinkedList.element(LinkedList.java:476) at Consumer.removeListElement(Consumer.java:29) at Consumer.run(Consumer.java:20)

Answer 1

Your exception is rather simple to explain. In

        while(!sharedList.isEmpty())
        {
            removeListElement();
        }

sharedList.isEmpty() happens outside of synchronization and so one consumer can still see a list as empty while another consumer has already taken the last element.

The consumer that wrongfully believed it is empty will not try to remove an element that is no longer there which leads to your crash.

If you want to make it threadsafe using a LinkedList you'll have to do every read / write operation atomic. E.g.

while(!this.isInterrupted())
{
    if (!removeListElementIfPossible())
    {
        break;
    }
}

and

// method does not need to be synchronized - no thread besides this one is
// accessing it. Other threads have their "own" method. Would make a difference
// if this method was static, i.e. shared between threads.
private boolean removeListElementIfPossible()
{
    synchronized(sharedList)
    {
        // within synchronized so we can be sure that checking emptyness + removal happens atomic
        if (!sharedList.isEmpty())
        {
            int removedItem = (Integer) (sharedList.element());
            sharedList.remove();
            System.out.println("Thread " + this.name + ": " + removedItem + " removed");
        } else {
            // unable to remove an element because list was empty
            return false;
        }
    }
    try {
        sleep(1000);
    } catch (InterruptedException e) {
        this.interrupt();
    }
    // an element was removed
    return true;
}

The same problem exists within your producers. But they would just create a 110th element or something like that.

A good solution to your problem would be using a BlockingQueue. See the documentation for an example. The queue does all the blocking & synchronization for you so your code does not have to worry.

Edit: regarding 2 while loops: You don't have to use 2 loops, 1 loop loops enough but you'll run into another problem: consumers may see the queue as empty before the producers have filled it. So you either have to make sure that there is something in the queue before it can be consumed or you'll have to stop threads manually in other ways. You thread.sleep(1000) after starting the producer should be rather safe but threads are not guaranteed to be running even after 1 second. Use e.g. a CountDownLatch to make it actually safe.

Answer 2

I am wondering why you are not using the already existing classes that Java offers. I rewrote your program using those, and it becomes much shorter and easier to read. In addition the lack of synchronized, which blocks all threads except for the one who gets the lock (and you even do double synchronization), allows the program to actually run in parallel.

Here is the code:

Producer:

public class Producer implements Runnable {

  protected final String name;
  protected final LinkedBlockingQueue<Integer> sharedList;
  protected final Random random = new Random();

  public Producer(final String name, final LinkedBlockingQueue<Integer> sharedList) {
    this.name = name;
    this.sharedList = sharedList;
  }

  public void run() {
    try {
      while (Thread.interrupted() == false) {
        final int number = random.nextInt(100);
        sharedList.put(number);
        System.out.println("Thread " + this.name + ": " + number);
        Thread.sleep(100);
      }
    } catch (InterruptedException e) {
    }
  }
}

Consumer:

public class Consumer implements Runnable {

  protected final String name;
  protected final LinkedBlockingQueue<Integer> sharedList;

  public Consumer(final String name, final LinkedBlockingQueue<Integer> sharedList) {
    this.name = name;
    this.sharedList = sharedList;
  }

  public void run() {
    try {
      while (Thread.interrupted() == false) {
        final int number = sharedList.take();
        System.out.println("Thread " + name + ": " + number + " taken.");
        Thread.sleep(100);
      }
    } catch (InterruptedException e) {
    }
  }
}

Main:

public static void main(String[] args) throws InterruptedException {
  final LinkedBlockingQueue<Integer> sharedList = new LinkedBlockingQueue<>(100);
  final ExecutorService executor = Executors.newFixedThreadPool(4);

  executor.execute(new Producer("producer", sharedList));
  Thread.sleep(1000);

  executor.execute(new Consumer("consumer1", sharedList));
  executor.execute(new Consumer("consumer2", sharedList));

  Thread.sleep(1000);
  executor.shutdownNow();
}

There are several differences:

• Since I use a concurrent list, I do not have to care (much) about synchronization, the list does that internally.

• As this list uses atomic locking instead of true blocking via synchronized it will scale much better the more threads are used.

• I do set the limit of the blocking queue to 100, so even while there is no check in the producer, there will never be more than 100 elements in the list, as put will block if the limit is reached.

• I use random.nextInt(100) which is a convenience function for what you used and will produce a lot less typos as the usage is much clearer.

• Producer and Consumer are both Runnables, as this is the preferred way for threading in Java. This allows to later on wrap any form of thread around them for execution, not just the primitive Thread class.

• Instead of the Thread, I use an ExecutorService which allows easier control over multiple threads. Thread creation, scheduling and other handling is done internally, so all I need to do is to choose the most appropriate ExecutorService and call shutdownNow() when I am done.

• Also note that there is no need to throw an InterruptedException into the void. If the consumer/producer is interrupted, that is a signal to stop execution gracefully as soon as possible. Unless I need to inform others "behind" that thread, there is no need to throw that Exception again (although no harm is done either).

• I use the keyword final to note elements that won't change later on. For once this is a hint for the compiler that allows some optimizations, it as well helps me to prevent an accidental change of a variable that is not supposed to change. A lot of problems can be prevented by not allowing variables to change in a threaded environment, as thread-issues almost always require something to be read and written at the same time. Such things cannot happen if you cannot write.

Spending some time to search through the Java library for the class that fits your problem the best usually solves a lot of trouble and reduces the size of the code a lot.

Answer 3

Try to switch places of

while(!sharedList.isEmpty())

and

synchronized(sharedList)

I don't think you need synchronized on removeListElement().