Synchronized Non-Final List

Question

Ok so as I understand it, its best to create a final static Object that I use for synchronization.

However, I also read that if the object reference doesn't change, then it won't have concurrency issues.

Do the following code violate synchronicity?

class Foo {
    private static ArrayList<Client> clients = null;

    public Foo() {
        clients = new ArrayList<>();
        //add stuff to list here..
    }

    public void addClient(Client C) {
        synchronized(clients) {
            clients.add(C);
        }
    }
}

Do I have to make clients final or create a final object if the clients ArrayList is never exposed directly (only other than through Getters)? In other words, I never provide a set method for the clients array so the reference never changes.

If the reference doesn't change, then why not make it final? — Zim-Zam O'Pootertoot, May 09 '13 at 19:00
I don't see an issue here, but why NOT just make it final anyway? — Nick, May 09 '13 at 19:01
I was thinking that final = const in C++ so that I can't add anything to it if I did that. But after googling it, I see its not the same. — Brandon, May 09 '13 at 19:02
i don't see any reason to use static here, and as stated above you'd better use final for it — hoaz, May 09 '13 at 19:03

score 3 · Accepted Answer · answered May 09 '13 at 19:01

3

Anyone creating an instance of Foo new Foo() overrides the clients array. it definitely is not thread safe

answered May 09 '13 at 19:01

Asaf

6,384
1
23
44

How :S It's a static array.. – Brandon May 09 '13 at 19:02
being static does not mean it can not be assigned. what you want is for it to be final. but if you make it static final than the above code will not compile, because you can not make 2 assignments to the same member ( first null than an actual array ) – Asaf May 09 '13 at 19:04

user949300 · Answer 2 · 2013-05-09T19:10:15.780

2

If you really want clients to be static, i.e., there is one list of Clients shared by all Foos so that it collects a huge list of all Clients, then you need to initialize it only once.

private static ArrayList<Client> clients = new ArrayList();

But I suspect that you want one client list per Foo, in that case, don't declare it static and, for clarity, do declare it final. (There are also some weird corner cases where you really must declare it final, as described in Java Concurrency in Practice.)

edited May 09 '13 at 19:10

answered May 09 '13 at 19:02

user949300

15,364
7
35
66

Yes :) I wanted one list for all Foos so I made it static. I initialized it like yours and made it final. I wasn't sure because in C++, I can only initialize in constructor or initialization list. Still learning java. That's a neat feature. Thanks! – Brandon May 09 '13 at 19:13

score 1 · Answer 3 · answered May 09 '13 at 19:10

To make it thread safe change the declaration of 'clients' like this:

private final static List<Client> clients = new ArrayList<Client>();

and use 'synchronized' as you already do.

In case you are confused of what the static keyword means: The static keyword means that the same instance of the 'clients' will be shared for all instances of Foo. If you remove the static keyword, each Foo instance will have it's own instance of 'clients'.

The final keyword simply prevents you from reassigning the clients variable.

Synchronized Non-Final List

3 Answers3