Is this Double-Checked Locking that is not Broken?

Question

This article explains "Double-Checked Locking" where the idea is to reduce lock contention. As the article explains it does not work. See the code sample in the table "(Still) Broken multithreaded version "Double-Checked Locking" idiom".

Now I think I found a variant that should work. Question is whether that is correct. Let's say we have a consumer and a producer that exchange data through a shared queue:

class Producer {
     private Queue queue = ...;
     private AtomicInteger updateCount;

     public void add(Data data) {
         synchronized(updateCount) {
             queue.add(task);
             updateCount.incrementAndGet();
         }
     }
}

class Consumer {
    private AtomicInteger updateCount = new AtomicInteger(0);
    private int updateCountSnapshot = updateCount.get();

    public void run() {
        while(true) {
            // do something
            if(updateCountSnapshot != updateCount.get()) {
                // synchronizing on the same updateCount 
                // instance the Producer has
                synchronized(updateCount) { 
                    Data data = queue.poll()
                    //  mess with data
                    updateCountSnapshot = updateCount.get();
                }
            }
        }
    }
}

Question now is whether you think this approach works. I'm asking to be sure, because tons of things would break if it doesn't ... The idea is to reduce lock contention when only entering a synchronized block in the consumer when the updateCount has changed in the meanwhile.

Answer 1

I suspect you are looking more for a Code Review.

You should consider the following:

• This is not double-checked locking.
• Your consumer will spin on nothing and eat cpu while no data is arriving.
• You use an AtomicInteger as a Semaphore.
• A BlockingQueue will do all of this for you.
• You haven't properly ensured that updateCount is shared.
• You do not have to synchronize on atomics.

Here's a simple Producer/Consumer pair for demonstration.

public class TwoThreads {

    public static void main(String args[]) throws InterruptedException {
        System.out.println("TwoThreads:Test");
        new TwoThreads().test();
    }

    // The end of the list.
    private static final Integer End = -1;

    static class Producer implements Runnable {

        final Queue<Integer> queue;

        public Producer(Queue<Integer> queue) {
            this.queue = queue;
        }

        @Override
        public void run() {
            try {
                for (int i = 0; i < 1000; i++) {
                    queue.add(i);
                    Thread.sleep(1);
                }
                // Finish the queue.
                queue.add(End);
            } catch (InterruptedException ex) {
                // Just exit.
            }
        }

    }

    static class Consumer implements Runnable {

        final Queue<Integer> queue;

        public Consumer(Queue<Integer> queue) {
            this.queue = queue;
        }

        @Override
        public void run() {
            boolean ended = false;
            while (!ended) {
                Integer i = queue.poll();
                if (i != null) {
                    ended = i == End;
                    System.out.println(i);
                }
            }
        }

    }

    public void test() throws InterruptedException {
        Queue<Integer> queue = new LinkedBlockingQueue<>();
        Thread pt = new Thread(new Producer(queue));
        Thread ct = new Thread(new Consumer(queue));
        // Start it all going.
        pt.start();
        ct.start();
        // Wait for it to finish.
        pt.join();
        ct.join();
    }

}