Is this unsynchronized object pool implementation good/safe?

Question

Can this object pool cause visibility problems with multiple threads? I'm specifically wondering about this kind of execution sequence:

Thread A - obtainObject()
Thread A - modifies object (lets say visibleState = 42)
Thread A - releaseObject()
Thread B - obtainObject() (get the object just released by A)
Thread A - does something unrelated or dies
Thread B - modifies object (lets say visibleState = 1)
Thread B - print objects visibleState
Thread B - releaseObject()

Could the modification from Thread A possibly become visible to Thread B after B has modified the state itself? (I know it doesn't occur in practice, but I can't figure out if and how the JLS/Javadoc guarantees this).

Here's the code, stripped down to only show the essentials. I left out generification and a factory to create objects.

import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicReference;

public class ObjectPool {

/** Maximum number of objects to be kept in pool */
int maxPoolSize = 10;

/** First object in pool (LIFO queue) */
final AtomicReference<PoolableObject> firstObject = new AtomicReference<PoolableObject>();

/** How many objects are currently in the pool */
final AtomicInteger poolSize = new AtomicInteger();

/** Gets an object from the pool. If no object is available
 * from the pool, a new object is created and returned */
public PoolableObject obtainObject() {
    while (true) {
        PoolableObject object = firstObject.get();
        if (object == null)
            break;
        if (firstObject.compareAndSet(object, object.next)) {
            poolSize.decrementAndGet();
            return object;
        }
    }
    // no more objects in pool, create a new object
    return new PoolableObject();
}

/** Returns an object to the pool. */
public void releaseObject(final PoolableObject object) {
    while (true) {
        if (poolSize.get() >= maxPoolSize)
            break;
        final PoolableObject first = firstObject.get();
        object.next = first;
        if (firstObject.compareAndSet(first, object)) {
            poolSize.incrementAndGet();
            break;
        }
    }
}

}

The objects managed by the pool are supposed to inherit from this class:

public class PoolableObject {

/** Links objects in pool in single linked list. */
PoolableObject next;

public int visibleState;

}

Do you refer to the comment on firstObject? Maybe my english fails me, but I think a single linked list can be reasonably called a 'queue'. — Durandal, Apr 23 '12 at 17:49
Perhaps, but it's not being used like a queue. An [`ArrayBlockingQueue`](http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/ArrayBlockingQueue.html) of size 1 would be much easier to use correctly, or perhaps a [`SynchronousQueue`](http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/SynchronousQueue.html). — Louis Wasserman, Apr 23 '12 at 18:01
You lost me here. That firstObject 'points' to the first object of a list, it can be of arbitrary length. — Durandal, Apr 23 '12 at 18:15
Ah. I see, I guess. Honestly, though...concurrency is an area where you should bend over backwards to use pre-built, already-tested abstractions, because it's _so_ hard to be sure that you've got it right. [`ConcurrentLinkedQueue`](http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/ConcurrentLinkedQueue.html) does the same thing you appear to be trying to do, but it's been exhaustively tested and optimized, and its locking properties have proofs attached. — Louis Wasserman, Apr 23 '12 at 18:25

score 0 · Accepted Answer · answered Apr 23 '12 at 17:57

As far as visibility goes, AtomicReference has the same memory barrier properties as a volatile variable.

Among those properties are that all actions of a thread before a write to a volatile variable "happen-before" that write. Or, to put it another way, if source code shows an action happening before a volatile write, that action can't be re-ordered by the JITC to occur after the volatile write. Thus, the scenario cited in the question won't be a problem; changes to the object made before it is released will be visible to other threads that subsequently capture the object.

However, I don't completely follow the way this pool is supposed to work, and there could be other problems. In particular, I'm not convinced that the lack of atomicity between firstObject and poolSize is safe. Threads can definitely see these variables in an inconsistent state, because they are not synchronized; I can't tell if that matters though.

Ah, I did miss the 'volatile' barrier property of AtomicReference. So in essence, any memory modifications that A performed before accessing the AtomicReference are guaranteed to be visible to B after its access to the AtomicRef. That perfectly clears up my question.As for the poolSize, yes its not perfectly synched with the real pooled object list. For my purposes, I consider that an unavoidable inaccuray, but the only effect of that should be that the set pool size is not obeyed accurately, which doesn't bother me. — Durandal, Apr 23 '12 at 18:13
@Durandal: Possibility of inconsistencies between `firstObject` and `poolSize` can be eliminated by using `AtomicStampedReference` to store both reference and size. — axtavt, Apr 23 '12 at 18:23

Is this unsynchronized object pool implementation good/safe?

1 Answers1