Atomic references are unnecessary when using synchronized

Question

This code came from the book Java Concurrency Guidelines by Fred Long. I understand that a group of atomic operations is not an atomic operation. So the code below is non-compliant. To find the code, please take a look at page 23.

public class Adder {

    private AtomicReference<BigInteger> first;
    private AtomicReference<BigInteger> second;

    public Foo(BigInteger f, BigInteger s) {
        first = new AtomicReference<BigInteger>(f);
        second = new AtomicReference<BigInteger>(s);
    }

    public void update(BigInteger f, BigInteger s) {
        first.set(f);
        second.set(s);
    }

    public BigInteger add() {
        return first.get().add(second.get());
    }
}

And the right solution looks like this:

final class Adder {
    // ...
    public synchronized void update(BigInteger f, BigInteger s){
        first.set(f);
        second.set(s);
    }

    public synchronized BigInteger add() {
        return first.get().add(second.get());
    }
}

But I think the atomic references in the correct solution are redundant because synchronized guarantees both visibility and atomicity.

So my solution would look like this:

public class Addrer {

    private BigInteger first;
    private BigInteger second;

    public Addrer(BigInteger f, BigInteger s) {
        first = f;
        second = s;
    }

    public synchronized void update(BigInteger f, BigInteger s) {
        first = f;
        second = s;
    }

    public synchronized BigInteger add() {
        return first.add(second);
    }
}

Am I right?

If you made the Adder class immutable (like BigInteger or String) you wouldn't need any synchronization at all, and no volatile or AtomicReference either. — Tesseract, May 02 '15 at 19:46

Piotrek Bzdyl · Accepted Answer · 2015-05-02T21:50:00.167

0

You need to make your first and second fields private and expose those values as synchronized methods. Otherwise reading the fields directly might result in outdated or partially outdated data from BigInteger objects (non-volatile field reads are not thread safe). Then your class will be thread safe.

You might try to make those fields volatile but it won't guarantee atomicity of your update or add methods as one thread might update one field just in the middle of your update or add method execution in another thread.

public class Adder {
    private BigInteger first;
    private BigInteger second;

    public Adder(BigInteger f, BigInteger s) {
        first = f;
        second = s;
    }

    public synchronized BigInteger getFirst() {
        return first;
    }

    public synchronized BigInteger getSecond() {
        return second;
    }

    public synchronized void update(BigInteger f, BigInteger s) {
        first = f;
        second = s;
    }

    public synchronized BigInteger add() {
        return first.add(second);
    }
}

edited May 02 '15 at 21:50

answered May 02 '15 at 19:20

Piotrek Bzdyl

12,965
1
31
49

I made them private. And yes, if I needed getters, they would be synchronized. – Maksim Dmitriev May 02 '15 at 19:23
@maraca I think the question author updated the question while or after I wrote my answer. – Piotrek Bzdyl May 02 '15 at 19:28
@maraca [Here](http://stackoverflow.com/posts/30005395/revisions) you can check when and what has been changed in the question and compare with my answer. Let's the question author and the rest of the community decide which answer is the best. – Piotrek Bzdyl May 02 '15 at 19:48
1

@maraca Could you elaborate more what is wrong with my answer? What is incorrect (especially in terms of thread safety)? – Piotrek Bzdyl May 02 '15 at 19:52
Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/76783/discussion-between-piotrek-bzdyl-and-maraca). – Piotrek Bzdyl May 02 '15 at 19:56

Atomic references are unnecessary when using synchronized

1 Answers1