Java method synchronization is not working as expected

Question

I'm trying to implement object pool design pattern. I've created the data structure as below

public class ObjectPool {
    private final Queue<PooledObject> objects;
    private final List<PooledObject> inUseObjects;

    public ObjectPool(final int poolSize) throws IllegalArgumentException {
        if (poolSize <= 0) {
            throw new IllegalArgumentException("Invalid pool size!");
        }

        objects = new ArrayDeque<PooledObject>(poolSize);
        inUseObjects = new ArrayList<PooledObject>(poolSize);

        for (int i = 0; i < poolSize; i++) {
            objects.add(new PooledObject());
        }
    }

    public synchronized PooledObject getObject() {
        PooledObject object;
        while ((object = objects.poll()) == null) {
            try {
                wait();
            } catch (final InterruptedException exception) {
                System.out.println(exception.getMessage());
            }
        }

        inUseObjects.add(object);

        return object;
    }

    public synchronized void returnObject(final PooledObject object) {
        if (object == null) {
            return;
        }

        inUseObjects.remove(object);
        objects.add(object);

        notifyAll();
    }
}

and also the runnable client

public class Client implements Runnable {
    private final ObjectPool pool;

    private final int id;

    private static int number = 0;

    public Client(final ObjectPool pool) {
        this.id = ++number;
        this.pool = pool;
    }

    private PooledObject getObjectFromPool() {
        PooledObject object = pool.getObject();
        System.out.println(this + " gets " + object);

        return object;

    }

    private void returnObjectToPool(final PooledObject object) {
        pool.returnObject(object);
        System.out.println(this + " returns " + object);
    }

    @Override
    public void run() {
        PooledObject object = getObjectFromPool();

        try {
            Thread.sleep(0);
        } catch (final InterruptedException exception) {
            System.out.println(exception.getMessage());
        }

        returnObjectToPool(object);
    }

    @Override
    public String toString() {
        return "User #" + id;
    }
}

I'm trying to run this code like this:

ObjectPool pool = new ObjectPool(2);

ExecutorService es = Executors.newCachedThreadPool();
for (int i = 0; i < 5; i++) {
    es.execute(new Thread(new Client(pool)));
}
es.shutdown();
try {
    if (es.awaitTermination(1, TimeUnit.MINUTES)) {
        System.out.println("Object pool pattern implemented!");
    }
} catch (InterruptedException exception) {
    System.out.println(exception.getMessage());
}

The problem is that the synchronization doesn't work as expected. As I far as I understand synchronized keyword's purpose is: It makes it possible to synchronize other blocks on the same object, so that if you have two blocks of code that may change the state of the same object, they don't interfere with each other. But when I run the program It can be clearly seen from the output that two threads gets same object from pool

output:

User #1 gets Object #1
User #2 gets Object #2
User #3 gets Object #2
User #5 gets Object #2
User #4 gets Object #1
User #3 returns Object #2
User #4 returns Object #1
User #2 returns Object #2
User #5 returns Object #2
User #1 returns Object #1

Answer 1

... when I run the program It can be clearly seen from the output that two threads gets same object from pool

This seems to me to be a classic race condition. Your Client threads call getObjectFromPool(), Thread.sleep(0), and returnObjectToPool(...). The sleep(0) is in effect a no-op so it is very possible for the first thread to get and return the object to the pool before another thread calls getObjectFromPool(). Also, your println(...) method happen after the fact which confuses the exercise. Both of these issues make it look like multiple threads are using the same object when this is not the case. Also, it is important to realize that System.out is a PrintStream which is a synchronized class so using it in a threaded program can affect the thread interleaving and is not recommended.

If you want to test to that your threads safely get, work on, and return the objects then you will need to increase the sleep time. Also, although you would never do this in a production object pool, moving the println(...) debug statements inside of the synchronized pool methods will show you what is going on with your thread pool code.

Lastly, for others, there are a number of good open source object pools already written including Apache Commons Pool.