Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I just wanted to confirm if this example I created would qualify as a good example of a multithreaded producer consumer. I would like any review changes on improving this example. I found that static fields are essential for synchronization of the threads. The wait-timeout mechanism along with the available flag functions as good as the wait/notifyAll mechanism. Synchronizing small blocks of code with private final static lock objects gives finer grained concurrency and prevents deadlocking.

package concurrency;

import java.util.ArrayList;
import java.util.List;

/*
 * A Producer Consumer example scenario. There are MAX_THREAD number of producers and consumers.
 * each thread produces and consumes a single element. 
 * Field transferCollection functions as the intermediatory collection between the producer and consumer.
 * Boolean field available acts as the signaling mechanism between the producer and consumer.
 * switched to true at the time when maximum number of threads have produced elements. 
 * 
 */
public class ProduceConsume {

    static int counter = 0;
    static List<Integer> transferCollection = new ArrayList<Integer>();
    static volatile boolean available = false;
    static int producer_sum, CONSUMER_SUM, MAX_THREADS = 1000;
    private static final Object addLock = new Object();
    private static final Object getLock = new Object();

    public static void main(String[] args) throws Exception {
        List<Runnable> producers = new ArrayList<Runnable>();

        Runnable producer = null;
        for (int i = 0; i < MAX_THREADS; i++) {
            producer = new Runnable() {
                ProduceConsume pc = new ProduceConsume();

                @Override
                public void run() {
                    pc.produce();
                }

            };
            producers.add(producer);
        }

        for (Runnable x : producers)
            new Thread(x).start();

        List<Runnable> consumers = new ArrayList<Runnable>();

        Runnable consumer = null;
        for (int i = 0; i < MAX_THREADS; i++) {
            consumer = new Runnable() {
                ProduceConsume pc = new ProduceConsume();

                @Override
                public void run() {
                    pc.consume();
                }

            };
            consumers.add(consumer);
        }

        for (Runnable x : consumers)
            new Thread(x).start();

    }

    /*
     * Consumer threads invoke this method when MAX_THREADS of the producer are finished producing and adding to the list the same 
     * number of items. 
     * <b>getLock</b>
     * A private final static lock object which threads wait on until the producer signals that elements are available on the list.
     *  <b>addLock</b>
     * A private final static lock object which threads wait on to sum up elements  in the transferCollection.
     */
    private void consume() {

        synchronized (getLock) {
            while (!available) {
                try {
                    getLock.wait(200);
                } catch (InterruptedException e) {
                    Thread.currentThread().interrupt();
                }
            }
        }

        if (counter > 0) {
            int size0 = transferCollection.size() - 1;

            synchronized (addLock) {
                int idx = --counter;
                CONSUMER_SUM += transferCollection.get(size0 - idx);
            }
        }
        if (counter == 0) {
            System.out.println("sum of consumer elements : " + CONSUMER_SUM);
            available=false;
        }

    }

    /*
     * Producer threads invoke this method till number of threads has reached MAX_THREADS which have produced same  
     * number of items. 
     *  <b>addLock</b>
     * A private final static lock object that allows thread to sequentially add elements to the collection and
     * produce a checksum at the end.
     */

    private void produce() {
        synchronized (addLock) {
            transferCollection.add(++counter);
            producer_sum += counter;
        }

        if (counter == MAX_THREADS) {
            System.out.println("sum of producer elements : " + producer_sum);
            available = true;
        }

    }

}
share|improve this question
add comment

2 Answers

up vote 1 down vote accepted

There are several visibility issues.

  1. Write operations on counter are protected by addLock. However, at least 3 read operations on this static field aren't protected by the same lock. Since the field is not volatile, the visibility of updates are not guaranteed at all.
  2. transferCollection.size() is not protected by addLock, so the size changes may not be visible for other threads.

Same issue exists for CONSUMER_SUM and producer_sum fields.

It may lead to:

  • livelock or resource starvation due to stale data;
  • logical errors due to instructions re-ordering;

Alternatively, consider using classes from java.util.concurrent package.

share|improve this answer
    
I thought that volatile should be used mainly with flag variables. If there were some visibility issues I wasn't able to notice it even after running it a number of times. How is it possible to recreate errors caused by non-volatile reads ? –  clinton Nov 15 '12 at 6:41
    
@clinton volatile is just a memory consistency mechanism in Java, it can be used not only for flags. To re-produce such errors you should run a load test in highly-concurrent environment for millions of iterations. It greatly depends on hardware, OS, etc. But such visibility issues are common, do not underestimate them. –  stoweesh Nov 20 '12 at 1:51
add comment

Why don't you use a BlockingQueue instead of the transfer collection? The implemenations in util.concurrent use internal synchronization, which is most likely more performant than external synchronization objects. Additionally your consumers can consume almost as fast as the producers produce, instead of waiting until all producer finished.

As the queue handles synchronization internally you could also remove the static variables and pass it to the producers and consumers on construction.

share|improve this answer
    
I didn't want to use any of the classes in the java.util.concurrent package as it they are examples of complete synchronizers. Mixing up synchronization methods in the Object class with java.util.concurrent package would make the class less cohesive. –  clinton Nov 12 '12 at 5:36
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.