Thread safe settings

Question

I'd like to design a settings class thread save. The settings have 2 attributes: String x and int y and should provide listener functionality to notify listener about changes. The problem is, how to make it thread safe.

Here is a complete code:

package org.example;

import java.util.Set;
import java.util.concurrent.CopyOnWriteArraySet;
import java.util.concurrent.locks.ReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock;

public final class Settings {
    private static final String DEFAULT_X = "<DEFAULT>";
    private static final int DEFAULT_Y = -1;

    public interface IListener {
        public void xChanged(final String newX, final String oldX);

        public void yChanged(final int newY, final int oldY);
    }

    // No access control via lock needed
    private final Set<IListener> listeners = new CopyOnWriteArraySet<>();

    /** Lock for read and write acces to {@link #x} and {@link #y} */
    private final ReadWriteLock lock = new ReentrantReadWriteLock();
    private String x = DEFAULT_X;
    private int y = DEFAULT_Y;

    public void addListener(final IListener listener) {
        if (listener == null) {
            throw new NullPointerException();
        }
        listeners.add(listener);
    }

    public void removeListener(final IListener listener) {
        if (listener == null) {
            throw new NullPointerException();
        }
        listeners.remove(listener);
    }

    public void copyFrom(final Settings origin) {
        if (origin == null) {
            throw new NullPointerException();
        }

        assert origin.getX() != null;
        lock.writeLock().lock();
        setX(origin.getX());
        setY(origin.getY());
        lock.writeLock().unlock();
    }

    public String getX() {
        lock.readLock().lock();
        final String x = this.x;
        lock.readLock().unlock();
        return x;
    }

    public void setX(final String newX) {
        if (newX == null) {
            throw new NullPointerException();
        }

        lock.writeLock().lock();
        if (!this.x.equals(newX)) {
            final String oldX = this.x;
            this.x = newX;
            lock.writeLock().unlock();
            // lock is released before the listeners get notified
            notifyXChanged(newX, oldX);
        } else {
            // nothing was changed
            lock.writeLock().unlock();
        }
    }

    public void setY(final int newY) {
        lock.writeLock().lock();
        if (this.y != newY) {
            final int oldY = this.y;
            this.y = newY;
            lock.writeLock().unlock();
            // lock is released before the listeners get notified
            notifyYChanged(newY, oldY);
        } else {
            // nothing was changed
            lock.writeLock().unlock();
        }
    }

    public int getY() {
        lock.readLock().lock();
        final int y = this.y;
        lock.readLock().unlock();
        return y;
    }

    private void notifyXChanged(final String newX, final String oldX) {
        for (final IListener listener : listeners) {
            listener.xChanged(newX, oldX);
        }
    }

    private void notifyYChanged(final int newY, final int oldY) {
        for (final IListener listener : listeners) {
            listener.yChanged(newY, oldY);
        }
    }
}

x and y are acess controlled, becouse getX, getY uses the read lock and setX, setY uses the write lock.

The interesting part is the copyFrom method. It must be atomic, i. e. x and y must set atomically. To get it atomically, I've wrapped it in a lock-unlock call:

    lock.writeLock().lock();
    setX(origin.getX());
    setY(origin.getY());
    lock.writeLock().unlock();

The problem is, that the listeners methods xChanged, yChanged are called so external code is executed. What about the listener call getY in xChanged?:

    settings.addListener(new IListener() {
        @Override
        public void yChanged(int newY, int oldY) { }

        @Override
        public void xChanged(String newX, String oldX) {
            System.out.println("y: " + settings.getY());
        }
    });

The write lock holds (from copyFrom), so the listener thread is blocked to aqquire the read lock (from getX). The result is a dead lock I think.

Another possible failure is, that the listener can throw an exception. When an exception is not handled, copyFrom returns abrupt without releasing the write lock. If is it handled (via try-catch), what should I do with this exception?

Are there best practices to handle such stateful classes with listener functionality (I have the same problem for a finite state machine etc.)?

Answer 1

Java isn't my first language, so bear with me if I confuse the syntax a bit. I agree that your copyFrom is not atomic in that the changes are externally visible before the copy is complete. I would also say that it isn't atomic because the origin is not locked and so could change while the copy occurs. Yes, I believe this is a possible point of deadlock - in general I believe it's best to always avoid firing events under a lock for this very reason.

If you just want to ensure that copyFrom is atomic in that sense that no intermediate state of the copy is visible externally, then you could do like this:

public void setX(final String newX) {
    if (newX == null) {
        throw new NullPointerException();
    }

    String oldX;

    lock.writeLock().lock();
    try {
        oldX = this.x;
        this.x = newX; // Assign new value
    }
    finally {
        lock.writeLock().unlock();
    }

    if (!this.x.equals(newX))
        notifyXChanged(newX, oldX);
}

// ... same for setY ...

public void copyFrom(final Settings origin) {
    if (origin == null) {
        throw new NullPointerException();
    }

    assert origin.getX() != null;

    String oldX;
    int oldY;

    // Acquire the data we need to perform the state change. 
    // Note that this isn't atomic (and wouldn't be even if it was in the 
    // lock). If we need it to be atomic then we need to implement an 
    // atomic getXandY method on the class.
    String origX = origin.getX();
    int origY = origin.getY();

    // Lock so we can do the state change
    lock.writeLock().lock();
    try {
        // Perform any state consistency checking here. It's still safe to 
        // throw an exception because we haven't changed any values yet.
        //...


        // Perform the state change. This should be simple operations that 
        // wont throw or lock
        oldX = this.x;
        oldY = this.y;
        this.x = newX; 
        this.y = newY;
    }
    finally {
        lock.writeLock().unlock();
    }

    // Notify anything that needs to know what happened
    notifyXChanged(newX, oldX);
    notifyYChanged(newY, oldY);
}

In terms of exception safety, notice that I follow every lock operation with a try..finally and a single unlock in the finally. This makes the code clear, and exception safe with regards to unlocking. In terms of exception safety regarding the internal consistency of the combined state of X and Y (for example if Y is always meant to be the string representation of X or something), you would perform consistency checks before any mutations but after entering the lock.

A general tip from my experience is to keep locks as short and simple as possible, not too many calls and calculations, and very simple flow control. You want to get the information you need to perform the full mutation, then lock, perform the mutation leaving the object in a perfectly consistent state at the end, and unlock asap. You can follow the same pattern for pretty much anything.

The part where I said there could be "consistency checking here" would be important if the combination of input and previous state does not make a valid mutation. For example in the case of a state machine you may want to verify that the state transition is valid and, if it isn't, throw an exception or simply return false or something (which are both safe at that point in the code). You can't do that before the lock because your checks may be working with data that changes under your feet, and you can't do it after changing part of the combined state because then aborting the mutation part way will leave the object in an inconsistent state.

I would also urge you to think clearly about the reason for the copy being atomic. Specifically, are X and Y coupled to each other in such a way that the object would contain inconsistencies if only X or only the Y was copied or changed? Think about the following:

• notifyXChanged and notifyYChanged are not under lock (and shouldn't be), which could introduce race conditions because the X and Y change notifications could be interleaved with different X and Y change notifications for the same object. If X and Y are inextricably linked then consider instead using notifySettingsChanged with a copy of all the fields or none.
• You are explicitly offering a setX and setY (as opposed to a combined setXAndY), which means the user of the object can break any internal consistency by writing to one and not the other.
• This is particularly apparent if, for example, you perform foo.copyFrom(bar); at the same time (different thread) as bar.setX("25"); bar.setY(42); - you could land up with foo.x being "<DEFAULT>" and foo.y being 42, which is means foo acquires a state that bar never had.