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.