Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I was writing my own implementation of BlockingQueue for practice. I am trying to avoid using the synchronized keyword for the methods. I would instead like to use ReentrantLock.

What is the best way to write this implementation? I am not a Java ninja and would greatly appreciate if someone could pinpoint the errors in my code here and suggest better ways of implementing it.

public class MyBlockingQueue<T> {

private Queue<T> queue;
private AtomicInteger limit = new AtomicInteger(10);
private Lock put_lock = new ReentrantLock();
private Lock take_lock = new ReentrantLock();
private Condition put_condition = put_lock.newCondition();
private Condition take_condition = take_lock.newCondition();

public MyBlockingQueue(AtomicInteger limit){
    queue = new LinkedList<T>();
    this.limit = limit;
}

public boolean put(T item) throws InterruptedException{
    put_lock.lockInterruptibly();
    try {
        while(queue.size() == limit.get()) {
            put_condition.await();
        }
        put_condition.signal();
        queue.add(item);
    } finally{
        put_lock.unlock();
    }

    return true;
}

public T take() throws InterruptedException{
    take_lock.lockInterruptibly();
    try {
        while (queue.size() == 0) {
            take_condition.await();
        }
        take_condition.signal();
        return queue.poll();
    } finally {
        take_lock.unlock();
    }
}
share|improve this question
add comment

1 Answer

These seem mixed up:

    while(queue.size() == limit.get()) {
        put_condition.await();
    }
    put_condition.signal();

and

    while (queue.size() == 0) {
        take_condition.await();
    }
    take_condition.signal();

when putting if queue is full you wait until someone takes and when taking if queue is empty you wait until someone puts.

You should have test cases with 11 puts followed by 1 take. and 1 take followed by 1 put. To see they do not deadlock.

What do you know, the example code for Condition is actually Bounded buffer. And you can see it here: http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/locks/Condition.html

But I like their condition names better;)

Limit is a constant as it is, hence does not need atomic integer or any other synchronization. Just declare it final. best way of making something threadsafe is to make it immutable.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.