views:

312

answers:

6

Reading this DZone article about Java concurrency I was wondering if the following code:


    private volatile List list;
    private final Lock lock = new ReentrantLock();

    public void update(List newList) {
        ImmutableList l = new ImmutableList().addAll(newList);
        lock.lock();
        list = l;
        lock.unlock();
    }

    public List get() {
        return list;
    }

is equivalent to:


    private volatile List list;

    public void update(List newList) {
        ImmutableList l = new ImmutableList().addAll(newList); 
        list = l;
    }

    public List get() {
        return list;
    }

The try { } finally { } block was omitted for brevity. I assume the ImmutableList class to be a truly immutable data structure that holds its own data, such as the one provided in the google-collections library. Since the list variable is volatile and basically what's going on is a copy-on-the-fly, isn't it safe to just skip on using locks?

+5  A: 

In this very specific example, I think you would be OK with no locking on the variable reassignment.

In general, I think you are better off using an AtomicReference instead of a volatile variable as the memory consistency effects are the same and the intent is much clearer.

Kevin
A: 

I think the default synchronization behavior of volatile doesn't guarantee the ReentrantLock behavior, so it might help with performance. Otherwise, I think it's fine.

Jon
+1  A: 

After re-reading this yes they are equivalent.

Clint
What is the problem with another thread making a copy of newList before locking the reference to list?
Kevin
@Clint following on Kevin's question, since l is local to the method and the update to list is protected, wouldn't a thread calling get() still get a consistent result?
teto
@Teto yes. The member variable list isn't being modified, only the reference is being updated.
Kevin
@Clint update() simply updates the list reference to newList. It doesn't add the contents of newList to the current list
Kevin
@Clint your logic is right but it doesn't represent the case in question, as list's contents are discarded and replaced completely with every update(). The new values don't depend at all on the old ones.
teto
+3  A: 

Yes, both of those code samples behave the same way in a concurrent environment. Volatile fields are never cached thread-locally, so after one thread calls update(), which replaces the list with a new list, then get() on all other threads will return the new list.

But if you have code which uses it like this:

list = get()
list = list.add(something) // returns a new immutable list with the new content
update(list)

then it won't work as expected on either of those code examples (if two threads do that in parallel, then the changes made by one of them may be overwritten by the other). But if only one thread is updating the list, or the new value does not depend on the old value, then no problem.

Esko Luontola
get() returns an ImmutableList, as I stated in the description of the problem, trying to modify it will only raise an exception.
teto
It's always possible to create a new immutable list which has the elements of the old immutable list plus some more - see in my comment how the add() method returns a new list. That's how it's done in functional programming. (If your example uses google-collections' ImmutableList, then it might not have such an operation, but every functional programming language's libraries do have.)
Esko Luontola
@Esko The original article does [now] update the list. This is part of the reason why synchronisation on the likes of collections rarely does what you want. / The example in the article was simple enough to be easily replaced by a cas loop.
Tom Hawtin - tackline
+1  A: 

If we are talking about timing and memory visibility. A volatile read is very close to the time it takes to do a normal read. So if you are doing get() alot then there is little difference. The time it takes to do a volatile write is about 1/3 time to acquire and release a lock. So your second suggestion is a bit faster.

The memory visibility as most people suggested is equivalent, that is any read before the lock acquisition happens before any write after the lock acquisition similar to any read before a volatile read happens before any subsequent write

John V.
+1  A: 

The following criteria must be met for volatile variables to provide the desired thread-safety:

1.Writes to the variable do not depend on its current value. 2.The variable does not participate in invariants with other variables.

Since both are met here - code is thread-safety

Loop