1

From Java Concurrency In Practice:

package net.jcip.examples;

import java.util.concurrent.atomic.*;

/**
 * NumberRange
 * <p/>
 * Number range class that does not sufficiently protect its invariants
 *
 * @author Brian Goetz and Tim Peierls
 */


public class NumberRange {

    // INVARIANT: lower <= upper
    private final AtomicInteger lower = new AtomicInteger(0);
    private final AtomicInteger upper = new AtomicInteger(0);

    public void setLower(int i) {
        // Warning -- unsafe check-then-act
        if (i > upper.get())
            throw new IllegalArgumentException("can't set lower to " + i + " > upper");
        lower.set(i);
    }

    public void setUpper(int i) {
        // Warning -- unsafe check-then-act
        if (i < lower.get())
            throw new IllegalArgumentException("can't set upper to " + i + " < lower");
        upper.set(i);
    }

    public boolean isInRange(int i) {
        return (i >= lower.get() && i <= upper.get());
    }
}

It says “Both setLower and setUpper are check-then-act sequences, but they do not use sufficient locking to make them atomic. If the number range holds (0, 10), and one thread calls setLower(5) while another thread calls setUpper(4), with some unlucky timing both will pass the checks in the setters and both modifications will be applied. The result is that the range now holds (5, 4) an invalid state.”

How could it happen if AtomicIntegers are thread-safe, did I miss some points? And how to fix this?

Palec
  • 12,743
  • 8
  • 69
  • 138
das kinder
  • 1,180
  • 2
  • 10
  • 16
  • 6
    probably because the atomicintegers are safe to themselves, but you're mixing operations of two independent atomicintegers. fetching from one and then setting on the other. – Marc B Feb 18 '14 at 14:58

2 Answers2

1

The involvement of AtomicInteger has nothing to do with the thread safety of your question.

Here is the problem:

  1. if (i > upper.get)
  2. lower.set(i);

steps 1 and 2 may be individually atomic, but together they form a two step, non-atomic, action.

Here is what can happen:

  1. if blammy passes
  2. context switch to another thread.
  3. the other thread calls upper.set(q) such that q < i
  4. context switch back to this thread.
  5. lower is set to i.

each individual step is atomic in nature, but the collection of steps is not atomic.

A java solution for this is:

  1. synchronized(some_object_reference, maybe this)
  2. {
  3. if (i > upper.get)
  4. lower.set(i)
  5. }

Be sure to us the same object reference to synchronize all setting of the upper and lower values.

DwB
  • 37,124
  • 11
  • 56
  • 82
0

Lets create an object:

NumberRange nr= new NumberRange();

Thread A:

nr.setLower(-1); //A1

Thread B:

nr.setLower(-3); //B1
nr.setUpper(-2); //B2

Execution order: B1, then A1 and B2 at the same time: If thread B pass the check before A (-3 < -2), and then A pass its check before B sets the value (-1 < 0) , this code won't throw any error because your methods are not atomic. The check is atomic, and the set method too, but together you have 2 atomic steps, not one.

Pablo Lozano
  • 10,122
  • 2
  • 38
  • 59