2

I am trying to implement Object Pool which has fixed number of Objects to be available for pool and using wait if pool if empty and notify when a thread releases an object.

I am able to achieve the above required functionality using below program.

I want to know if the implementation is correct or needs any modification from Interview point of view ?

    import java.util.ArrayList;
import java.util.List;

class ObjectPool
{
    static List objects = new ArrayList();
    static
    {
        objects.add("Object One");
        objects.add("Object Two");
    }
    public Object getObject()
    {
            synchronized(objects)
            {
                if(objects.isEmpty())
                {
                    System.out.println(Thread.currentThread().getName()  + " waiting as Object Pool is empty");
                    try {
                        objects.wait();
                        System.out.println(Thread.currentThread().getName()  + " Got Notification");
                    } catch (InterruptedException e) {
                        // TODO Auto-generated catch block
                        e.printStackTrace();
                    }
                }
                Object locked = objects.get(objects.size()-1);
                objects.remove(locked);
                System.out.println(Thread.currentThread().getName()  + " got lock of object : "+ locked);
                return locked;
        }
    }

    public boolean release(Object released)
    {
        synchronized(objects)
        {
        System.out.println(Thread.currentThread().getName() + " releasing Object : "+released);
        objects.notify();
        return objects.add(released);
        }
    }
}


    public class MainforObjectPool implements Runnable
    {
        static ObjectPool p = new ObjectPool();
        public static void main(String[] args)
        {
            MainforObjectPool m = new MainforObjectPool();

            Thread t1 = new Thread(m,"thread 1");
            Thread t2 = new Thread(m,"thread 2");
            Thread t3 = new Thread(m,"thread 3");
            Thread t4 = new Thread(m,"thread 4");
            Thread t5 = new Thread(m,"thread 5");



            t1.start();
            t2.start();
            t3.start();
            t4.start();
            t5.start();


            System.out.println("Main Thread Completed");


        }

        public void run()
        {
            Object locked = p.getObject();
            try {
                Thread.sleep(2000);
            } catch (InterruptedException e) {
                // TODO Auto-generated catch block
                e.printStackTrace();
            }

            p.release(locked);
        }
    }
  • I'll ask just in case: are you aware of freely available object pool implementations, e.g. [Apache Commons Pool](https://commons.apache.org/proper/commons-pool/)? Unless you're doing this as a kind of coding exercise, you should use an existing library. – Tomasz Linkowski Jul 20 '18 at 09:36
  • 1
    Yes, it needs lots of modification. Unless you've interviewing for a very junior role, this naive use of synchronization throws a massive red flag. And the `static` is **Just Wrong TM**. – Boris the Spider Jul 20 '18 at 09:36
  • Hi @BoristheSpider can you please share the modification required or any link form which i can use to understand the correct implementation, TM means ? – springcloudlearner Jul 20 '18 at 09:37
  • Sorry, but there is none of this code that is salvagable. I would suggest discarding it and learning how to use an [`ArrayBlockingQueue`](https://docs.oracle.com/javase/10/docs/api/java/util/concurrent/ArrayBlockingQueue.html). If you want to see how to implement a pool correctly, then Apache Commons Pool is [open source](https://github.com/apache/commons-pool). – Boris the Spider Jul 20 '18 at 09:40
  • Another thing - for a comprehensive code review, you could try [Code Review SE](https://codereview.stackexchange.com/). – Boris the Spider Jul 20 '18 at 10:01

1 Answers1

0

Long back ago, I solved the similar problem using an abstraction that is similar to yours. Fortunately, I saved it and put it here. For the sake of keeping this answer short and hopefully being clear in my words, I'll not be posting the whole code here.


Implementation:

You can have an abstract class that has a generic which can allow you to create the pools of your favorite objects. Yes, I'll use favorite like this and get away with it

This abstract class lets its implementors handle the logic of creating/expiring objects.

This class has a queue for objects that are locked and unlocked.

When you checkIn an object in the pool, it lands in the unlocked queue.

When you checkOut an object from the pool, it checks whether the object should be expired or not by calling an abstract method validate(). If this function returns true, the object is then moved to the locked queue. If this function returns false, then the object is removed from the pool and is expired by calling the abstract function expire() (you can use notify in this). If the object to be checked out is not being pooled, then it's created and put in the locked queue.


The code:

public abstract class ObjectPool<T> {
    private long expirationTime;

    private Hashtable<T, Long> locked, unlocked;

    public ObjectPool() {
        expirationTime = 30000; // 30 seconds
        locked = new Hashtable<T, Long>();
        unlocked = new Hashtable<T, Long>();
    }

    /**
     * Implemented in concrete class. Create an object to be pooled.
     */
    protected abstract T create();

    /**
     * Used to check whether the object should be kept in the lock, or released.
     */
    public abstract boolean validate(T o);

    /**
     * Object expired. (Use notify?)
     */
    public abstract void expire(T o);

    public synchronized T checkOut() {
        long now = System.currentTimeMillis();
        T t;
        if (unlocked.size() > 0) {
            Enumeration<T> e = unlocked.keys();
            while (e.hasMoreElements()) {
                t = e.nextElement();
                if ((now - unlocked.get(t)) > expirationTime) {
                    // object has expired
                    unlocked.remove(t);
                    expire(t);
                    t = null;
                } else {
                    if (validate(t)) {
                        unlocked.remove(t);
                        locked.put(t, now);
                        return (t);
                    } else {
                        // object failed validation
                        unlocked.remove(t);
                        expire(t);
                        t = null;
                    }
                }
            }
        }
        // no objects available, create a new one
        t = create();
        locked.put(t, now);
        return (t);
    }

    public synchronized void checkIn(T t) {
        locked.remove(t);
        unlocked.put(t, System.currentTimeMillis());
    }

    public synchronized long getExpirationTime() {
        return expirationTime;
    }

    public synchronized void setExpirationTime(long expirationTime) {
        this.expirationTime = expirationTime;
    }   

}

Jay
  • 1,089
  • 12
  • 29