Synchronized blocks and Locks

Question

I have two threads and I am currently doing locking using an Object's notify() and wait() methods inside Synchronized blocks. I wanted to make sure that the main thread is never blocked so I used a boolean this way (only relevant code provided.)

//Just to explain an example queue
private Queue<CustomClass> queue = new Queue();

//this is the BOOLEAN
private boolean isRunning = false;

private Object lock;

public void doTask(){
       ExecutorService service = Executors.newCachedThreadPool();

            //the invocation of the second thread!!
            service.execute(new Runnable() {
                @Override
                public void run() {
                       while(true){
                            if (queue.isEmpty()){
                                synchronized (lock){
                                     isRunning = false;   //usage of boolean
                                     lock.wait();
                                }
                            }
                            else{
                                process(queue.remove());
                            } 
                       }
                });

}

//will be called from a single thread but multiple times.
public void addToQueue(CustomClass custObj){


       queue.add(custObj);
       //I don't want blocking here!!
       if (!isRunning){
           isRunning = true;      //usage of BOOLEAN!     
           synchronized(lock){
           lock.notify();
           }
       }
}

Does anything seems wrong here? thanks. Edit: Purpose: This way when add() will be called the second time and more, it won't get blocked on notify(). Is there a better way to achieve this non blocking behavior of the main thread?

Mark `isRunning` as `volatile`. It is accessed from two threads. — Grzegorz Żur, Jun 17 '13 at 10:37
The problem is that `isRunning` could be cached by the worker thread running on a separate core and updated on that core but not pushed back to the core that the main thread is running on... `volatile` puts in a memory barrier that stops that happening. — selig, Jun 17 '13 at 10:43
isRunning is the variable you are changing inside the synchronized block and you are not blocking that, you are blocking some object instead, I don;t understand the purpose ! — voidMainReturn, Jun 17 '13 at 10:43
Also - Java doesn't implement closures properly i.e. I don't think this will compile as the anonymous inner class cannot access `isRunning`. `isRunning` should be set using a method call. — selig, Jun 17 '13 at 10:50
Why not use `BlockingQueue`? You don't have to lock & signal by yourself. — R Kaja Mohideen, Jun 17 '13 at 10:52

score 0 · Accepted Answer · answered Jun 17 '13 at 11:18

0

Although you do not show the addToQueue code I am fairly certain that this code will not work properly, as you are accessing the shared queue (which is not thread-safe) without any synchronization.

process(queue.remove());

Instead of trying to make your custom queue work (I doubt that your plan with the boolean flag is possible), save the time and work and use one of the BlockingQueues or ConcurrentLinkedQueue provided in the JDK.

answered Jun 17 '13 at 11:18

Pyranja

3,529
22
24

Yes. Now I intend to use LinkedBlockingQueue. – Abdullah Shoaib Jun 17 '13 at 11:27
Oi - shame on me : Forgot to scroll down in the code section. (. – Pyranja Jun 17 '13 at 11:58

score 0 · Answer 2 · answered Jun 17 '13 at 11:35

The Queue is not synchronized and therefore the above code can suffer from the lost wake-up call typical for conditional variables and monitors. https://en.wikipedia.org/wiki/Producer%E2%80%93consumer_problem For example, here is a problematic sequence: At the beginning of the run the Q is empty and isRunning is false. Thread 1 (t1) checks if Q is empty (which is true) and then stops running. Than Thread 2 (t2) starts running and execute the addToQ method. and then t1 continues running and waits on the lock although the Q is not empty. If you want a non-blocking solution you can use the non-blocking Q java is offering (http://docs.oracle.com/javase/1.5.0/docs/api/java/util/concurrent/ConcurrentLinkedQueue.html)Of course, you can use java own blockingQueue, but this is blocking.

Synchronized blocks and Locks

2 Answers2