current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 1 down vote favorite

This is a follow-up question to this question.

Is this a reasonable implementation for a blocking version of method ArrayBlockingQueue.drainTo(Collection)?

/**
 * This blocking method blends {@link #take()} with {@link #drainTo(Collection)}.
 * <br>
 * Chiefly, {@code take()} blocks and {@code drainTo(Collection)} does not block.
 */
public int drainAtLeastOneTo(Collection<? super E> c)
throws InterruptedException {
    final ReentrantLock lock = this.lock;
    lock.lockInterruptibly();
    try {
        while (count == 0) {
            notEmpty.await();
        }
        final int x = drainTo(c);
        return x;
    }
    finally {
        lock.unlock();
    }
}

To clarify: This code comes from the Sun JDK source code of ArrayBlockingQueue. I simply blended the source from take() with drainTo(Collection).

I am considering adding it to a modified version of ArrayBlockingQueue.

share|improve this question
    
What is notEmpty? How much of your code is a rewrite of ArrayBlockingQueue? Your lock lock is completely useless too, each time a thread calls the method they will get a new lock instance, so you have not implemented any locking at all..... your code does not work... though I suspect you did not know that when you posted it. –  rolfl Aug 28 at 4:17
    
I realize that your lock is a shadow-name of the instance lock. That's a bad practice - using the same names in methods variables as instance variables... my first comment above is "wrong". –  rolfl Aug 28 at 4:37

1 Answer 1

up vote 1 down vote

Your problem is easily solved without having to delve in to the internals of ArrayBlockingQueue, and without needing a new ReEntrantLock either. SImply use the methods that are currently available, and combine them....

public int drainAtLeastOneTo(Collection<? super E> c) 
        throws InterruptedException {
    c.add(take());
    return 1 + drainTo(c);
}
share|improve this answer
    
This is an interesting idea. I will consider it. However, your solution requires the caller to obtain (and free) the lock twice. Mine only requires it once. Why do I care? I have an situation where the producer is relatively much faster than the consumer. Thus, the consumer is sometimes starved on take(). My idea was to "take more" with this method. –  kevinarpe Aug 28 at 6:34
    
@kevinarpe then maybe a different queue is the answer –  ratchet freak Aug 28 at 8:20
    
@kevinarpe - about the double-kick at the lock can, yes, I lock it twice, but, for the record, you do too... just you nest yours instead. drainTo has an internal lock, and nesting lock blocks may not be free either. Will need a performance test to check if it has a significant impact. –  rolfl Aug 28 at 15:48

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.