What object should I lock on when I am passing a Collection<Foo> into a separate class ?

Question

Please refer to UML

The Connection class's constructor initializes its foos member via

foos = Collections.synchronizedList( new ArrayList<Foo>(10) );

When Connection#start() is invoked, it creates an instance of Poller (while passing the foos reference into Poller's constructor) & Poller is started (Poller is a Runnable).

Question: The Poller thread will add to & remove objects from the list based on external events. Periodically clients will invoke Connection#snapshot() to retrieve the list. Since the implementation within Poller will perform a check to avoid duplicates during additions, it is not thread safe.

e.g. implemention of Poller#run

if( _foos.indexOf( newFoo ) == -1 )
{
    _foos.add( newFoo );
}

What can I synchronized on in Connection as well as Poller to order to be thread safe ?

Answer 1

You might return a new List from snapshot():

public List<Foo> snapshot() {
    return new ArrayList<Foo>(foos);
}

Given that you're returning a "snapshot", it seems OK to me that the list is guaranteed to be up-to-date only at the moment it gets returned.

If you're expecting clients to add/remove members from foos, then you'd probably need to expose those operations as methods on Connection.

Answer 2

I'd take a look at CopyOnWriteArrayList as a replacement for the ArrayList in the example above. That way you won't need to synchronize on anything since you have a thread safe collection out of the box, so to speak...

http://java.sun.com/j2se/1.5.0/docs/api/java/util/concurrent/CopyOnWriteArrayList.html

From the API CopyOnWriteArrayList is...

A thread-safe variant of ArrayList in which all mutative operations (add, set, and so on) are implemented by making a fresh copy of the underlying array.

n.b. This is only a viable solution if the number of traversals outweigh the number of additions/updates to the collection. Is this the case?

Answer 3

There is a clean solution using interfaces and anonymous inner classes.

In the Connection class add the following:

public static interface FooWorker {
    void onFoos(List<Foo> list);
}

public synchronized void withFoosSafely(FooWorker worker) {
    worker.onFoos(foos);
}

In the Poller class do the following:

public void doWork() {
    connection.withFoosSafely(new FooWorker() {
         public void onFoos(List<Foo> list) {
              /// add, remove and change the list as you see fit
              /// everything inside this method is thread-safe
         }
    });
}

It requires a bit of additional code (no closures yet in Java) but it guarantees thread safety and also makes sure clients don't need to take care of locking - less potential bugs in the future.

Answer 4

Perhaps I am not getting the point, it seems Connection#snapshot should be synchronized on this (or on _foos) and so does the code block of Poller that manages Connection._foos.

What am I missing?