Reputation: 47885
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 synchronize on in Connection as well as Poller to order to be thread safe?
Upvotes: 4
Views: 207
Reputation: 28761
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?
Upvotes: 0
Reputation: 7261
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.
Upvotes: 1
Reputation: 90513
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
.
Upvotes: 0
Reputation: 63734
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?
Upvotes: 1