Does this basic Java object pool work?

Question

Does the following basic object pool work? I have a more sophisticated one based on the same idea (i.e. maintaining both a Semaphore and a BlockingQueue). My question is - do I need both Semaphore and BlockingQueue? Am I right that I don't need to do any synchronisation?

import java.util.Collection;
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.Semaphore;

public final class Pool<T> {

    private final BlockingQueue<T> objects;
    private final Semaphore permits;

    public Pool(Collection<? extends T> objects) {
        // we have as many permits as objects in our pool:
        this.permits = new Semaphore(objects.size());
        this.objects = new ArrayBlockingQueue<T>(objects.size(), false, objects);
    }

    public T borrow() {
        this.permits.acquireUninterruptibly();
        // we have a permit, so there must be one in there:
        return this.objects.poll();
    }

    public void giveBack(T object) {
        this.objects.add(object);
        this.permits.release();
    }
}

Answer 1

Maybe you should check that objects exists, that's the only thing I have.

Edit: I didn't read the code that carefully. So I edtied the post a bit. :(

Answer 2

Use take() instead of poll(), and put() instead of add(). The semaphore is then completely redundant so you can just get rid of it. But yes, that looks good.

Answer 3

As has been pointed out, a bounded BlockingQueue alone would be sufficient. For example, the following code will do what you want:

import java.util.Collection;
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.BlockingQueue;

public final class Pool<T> {

    private final BlockingQueue<T> objects;

    public Pool(Collection<? extends T> objects) {
        this.objects = new ArrayBlockingQueue<T>(objects.size(), false, objects);
    }

    public T borrow() throws InterruptedException {
        return this.objects.take();
    }

    public void giveBack(T object) throws InterruptedException {
        this.objects.put(object);
    }
}

Also, you might want to consider supporting a timed version of borrow() using BlockingQueue.poll().

If you didn't have a bounded blocking queue data structure, then you can impose a semaphore on top of any data structure to create a thread safe and bound behavior.

Answer 4

Its worth nothing that an ArrayBlockingQueue creates an object when you take an entry from it. So your pool won't actually save objects. It could only help if your objects are expensive to create.

Answer 5

A somewhat modified sjlee's example; allowing creation of expensive objects on demand. My case did not require any blocking facility hence I have replaced this with non-blocking queue type. As a benefit, there's no need to deal with InterruptedExceptions.

import java.util.Collection;
import java.util.Queue;
import java.util.concurrent.ConcurrentLinkedQueue;

public abstract class ObjectPool<T> {

    private final Queue<T> objects;

    public ObjectPool() {
        this.objects = new ConcurrentLinkedQueue<T>();
    }

    public ObjectPool(Collection<? extends T> objects) {
        this.objects = new ConcurrentLinkedQueue<T>(objects);
    }

    public abstract T createExpensiveObject();

    public T borrow() {
        T t;
        if ((t = objects.poll()) == null) {
            t = createExpensiveObject();
        }
        return t;
    }

    public void giveBack(T object) {
        this.objects.offer(object);   // no point to wait for free space, just return
    }
}

Answer 6

Maybe use a stack instead of a queue? This gives a chance of getting an object that is still sitting in the processor cache.