ExerciseChallenge
Challenge
Fix four broken guards
25 minintermediate2–12 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 StarterHints
Hint 1
For each class, ask the three questions in order: is anything half-done, is anything invisible, is anything reordered?
Hint 2
A synchronized instance method locks `this`. What does a synchronized static method lock?
Hint 3
counter++ on an Integer replaces the object. What happens to the monitor you were holding?
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?