views:

159

answers:

3

Hello,

I have a code piece that I am reviewing (using FindBug).

public class MyClass{
...
private BlockedQueue q = new LinkedBlockingQueue<MyData>(1000);
private static final batchSize = 1000;

public boolean testMethod(){
    boolean done = false;
    synchronized(q){
       if(q.size == batchSize){
         q.notify();
         done = true;
       }
    }
    return done;

}

When I run FindBug on this piece of code, it complains that -

This method performs synchronization an object that is an instance of a class from the java.util.concurrent package (or its subclasses). Instances of these classes have there own concurrency control mechanisms that are distinct from and incompatible with the use of the keyword synchronized.

If I comment out the synchronized code piece synchronized(q){, it complains -

This method calls Object.notify() or Object.notifyAll() without obviously holding a lock on the object. Calling notify() or notifyAll() without a lock held will result in an IllegalMonitorStateException being thrown

How would I implement this method so that it passes FindBugs validation? Is the above implementation right one for notification in cases for concurrent classes?

Thank you.

A: 

The first error is stating that you shouldn't use primitive synchronization controls on java.util.concurrent classes (like BlockingQueue).

Generally, this is good practice as they take care of the synchronization for you. I'd imagine that there is a better way to solve your problem at hand. What is the actual problem you're trying to solve?

The second error is caused by the fact that you MUST own the lock/monitor of an object (by sychronizing on it) to call wait/notify/notifyAll on it

Kevin
The problem I am trying to resolve is to ensure that FindBug does not report any error. The code is a marriage between concurrent classes and primitive synchronization. I would like to know what the best implementation is. As I said, I am "reviewing" the code, and looking at best possible implementation.
Win Man
I'm fairly certain that you're solving your problem in the wrong way, I'm just not sure what problem you're trying to solve. Can you edit your initial post to provide your complete solution?
Kevin
+2  A: 

notify() goes together with wait() and should not be used with classes of java.util.concurrent.

BlockingQueue uses internal mechanisms to block on a put() if there is no space for more elements or on poll() if there is no element to consume. You don't have to care about this.

tangens
A: 

BlockingQueue is a syncronizer object - coordinates the control flow of threads based on its state and thus control flow of producer/consumer threads because take and put block until the queue enters the desired state (not empty or not full).

Also good practice in concurrency programming assumes that wait and notify are placed in while loop.

Loop