Make singleton observer pattern synchronized?

Question

When I have a common singleton pattern like the one above: To make it thread save, I made the access to adding and removing listeners synchroinized on the list of the listeners. But is it important to do this with every access on every listener? For example, should the same think be done with the setPosition method?

public class Singleton {
    private static Singleton instance;
    private final List<ChangeListener> listeners = new ArrayList<>();
    private int position;

    private Singleton() {
    }

    public static synchronized Singleton getInstance() {
        if (instance == null) {
            instance = new Singleton();
        }
        return instance;
    }

    public int getPosition() {
        return position;
    }

    public void setPosition(int position) {
        this.position = position;
        for (ChangeListener l : listeners) {
            l.do(position);
        }
    }

    public void addChangeListener(ChangeListener listener) {
        synchronized (listeners) {
            listeners.add(listener);
        }
    }

    public void removeChangeListener(ChangeListener listener) {
        synchronized (listeners) {
            listeners.remove(listener);
        }
    }

    public interface ChangeListener {
        public void do(int a);
    }
}

If the constructor of your "Singleton" class does not take any params, you can create the instance like this: private static final Singleton INSTANCE = new Singleton(); and change, getInstance() { return INSTANCE}; — Anupam Saini, Feb 09 '15 at 10:18
Personally I would not create a Singleton class the way you are creating, it would be better if you use some sort of DI and will not create an instance the way i described in the last cmment. https://github.com/google/guice/wiki/Scopes — Anupam Saini, Feb 09 '15 at 10:23
The code above is just an abstraction. In my real code, I'm ititalizing some final variables by the constructor. — Paul Woitaschek, Feb 09 '15 at 10:26

score 2 · Answer 1 · edited May 23 '17 at 11:56

2

I would definitely make setPosition method synchronized, just because it is not a good idea to change the collection while you are iterating it in:

for (ChangeListener l : listeners) {
    l.do(position);
}

For more info, take a look here

edited May 23 '17 at 11:56

Community

1
1

answered Feb 09 '15 at 10:11

egelev

1,175
2
11
27

score 1 · Accepted Answer · answered Feb 09 '15 at 10:25

An alternative is to use something like java.util.concurrent.CopyOnWriteArrayList. This is thread safe but achieves this by taking a copy every time you write to the list. At first glance this may seem expensive but generally you will be adding/removing listeners infrequently and iterating over the list to call the listeners much more often. When used in this way it is generally more efficient that using explicit synchronisation.

Make singleton observer pattern synchronized?

2 Answers2