Challenge

Fix four broken guards

25 minintermediate212 yrs

Edge cases. You have to reason, and two valid fixes differ.

What this teaches

  • volatile on a mutable collection protects the reference, not the contents
  • A synchronized instance method and a synchronized static method lock different monitors
  • Locking on a reassigned or boxed field locks a different object each time
  • Two atomic fields do not make an atomic pair
  • Naming which of the three problems is violated is what makes the fix obvious

Starter

Starter.java
import java.util.*;
import java.util.concurrent.*;
import java.util.concurrent.atomic.*;

/**
 * Challenge: four classes, four different broken guards.
 *
 * Every one of them was written by someone who knew that concurrency needs
 * protection and picked the wrong protection. For each, name which of the
 * three problems is actually violated — atomicity, visibility, or ordering —
 * before you change a line. The fix follows from the name.
 *
 * One of the four is fixed by REMOVING a keyword.
 */
public class Starter {

    /* ─────────── 1 ─────────── */

    /**
     * The author made the field volatile so the map would be thread-safe.
     * Reads vastly outnumber writes; entries are replaced a few times a day.
     */
    static class Registry {
        private volatile Map<String, String> entries = new HashMap<>();

        void put(String key, String value) {
            entries.put(key, value);
        }

        String get(String key) {
            return entries.get(key);
        }
    }

    /* ─────────── 2 ─────────── */

    /**
     * Both methods are synchronized, and both touch `total`. The author
     * concluded that one thread can be inside at a time.
     */
    static class Metrics {
        private static long total = 0;

        synchronized void record(long value) {
            total += value;
        }

        static synchronized void reset() {
            total = 0;
        }

        static long total() {
            return total;
        }
    }

    /* ─────────── 3 ─────────── */

    /**
     * A lock, taken on the thing being protected — which sounds like exactly
     * the right instinct.
     */
    static class Hits {
        private Integer counter = 0;

        void hit() {
            synchronized (counter) {
                counter++;
            }
        }

        int count() {
            return counter;
        }
    }

    /* ─────────── 4 ─────────── */

    /**
     * Both fields are atomic, so every individual update is safe. The
     * invariant this class exists to maintain is that `count` is always the
     * number of values that went into `sum`.
     */
    static class RunningAverage {
        private final AtomicLong sum = new AtomicLong();
        private final AtomicInteger count = new AtomicInteger();

        void add(long value) {
            sum.addAndGet(value);
            count.incrementAndGet();
        }

        /** Must never see a sum from after the count, or vice versa. */
        double average() {
            int n = count.get();
            return n == 0 ? 0 : (double) sum.get() / n;
        }
    }

    /* ─────────── your work ─────────── */

    // TODO 1: for each of the four, write one line: which problem is violated,
    // and what the observable failure is. Do this before any edits.
    //
    //   1. Registry        problem: ______  failure: ______
    //   2. Metrics         problem: ______  failure: ______
    //   3. Hits            problem: ______  failure: ______
    //   4. RunningAverage  problem: ______  failure: ______

    // TODO 2: fix Registry. There are two correct answers depending on intent
    // — one uses a concurrent collection, one keeps volatile and changes what
    // is assigned to it. Given "reads vastly outnumber writes", pick the
    // second and say why.

    // TODO 3: fix Metrics. Name the two monitors involved before you decide.

    // TODO 4: fix Hits. Say what happens to the monitor after the first
    // increment, and separately why Integer values under 128 make it worse.

    // TODO 5: fix RunningAverage. Note that no combination of atomics can do
    // this, and say why in terms of what an invariant over two fields needs.

    // TODO 6: one of your four fixes made a field's volatile modifier
    // redundant. Remove it and explain what now provides the visibility.

    public static void main(String[] args) throws Exception {
        // TODO 7: write one hammer test per class that FAILS before your fix
        // and PASSES after. Three of the four can be made to fail reliably.
        // For the one that cannot, say why — and what that tells you about
        // testing concurrency.
        System.out.println("write the tests, then delete this line");
    }
}

Run it locally:

cd exercises/java/concurrency/volatile-vs-synchronized/02-challenge
javac Starter.java -d /tmp/out && java -cp /tmp/out Starter

Hints

  1. Hint 1

    For each class, ask the three questions in order: is anything half-done, is anything invisible, is anything reordered?

  2. Hint 2

    A synchronized instance method locks `this`. What does a synchronized static method lock?

  3. Hint 3

    counter++ on an Integer replaces the object. What happens to the monitor you were holding?

  4. Hint 4

    If two fields must agree at all times, no number of atomics will do it — you need one critical section over both.

Done when

  • All four classes are fixed, and each fix is the smallest one that works
  • Each has a comment naming which of the three problems it violated
  • One of the four is correctly fixed by removing a keyword, not adding one
  • You can justify each choice of atomic versus lock rather than defaulting

← Back to What does volatile guarantee, and what does it not?