Why does this threaded code hang

Question

In the below code, when I execute the producercon class, sometimes the execution stucks, looks like a deadlock. But if i make the get_flag () synchronized then there are no such problems.

I cannot figure out how there can be a problem. the flag can either true or false so only one of the producer or consumer will get into the if statement. After one of them enters the if it will enter the monitor with object r (both are initialized with the same object reference). The only problem which can happen that the r object being modified by the increment_decrement () function call, and the get_flag () reading the flag at the same time, but even then it will not enter the if in that iteration, but it will enter the if block on the next iteration, and even if the first thread did not leave the monitor, it will wait there for it (before the synchronized block).

How, and why is the program halting/hanging if get_flag () is not made synchronized ?

import java.io.*;

class resource
{
   private boolean res, flag;

   resource ()
   {
     flag=false;
   }

   boolean get_flag ()
   {
     return flag;
   }

   void increment_decrement (String s,boolean t)
   {
     res=t;
     flag=t;
      try 
      {
        System.out.print("\n"+s+":"+res);
        Thread.sleep(200);
      }
      catch(InterruptedException e)
      {
      }
   }
}

class producer implements Runnable
{
    resource r1;
    Thread t1;

    producer(resource r)
    {
      r1 = r;
      t1 = new Thread(this);
      t1.start();
    }

    public void run ()
    {  
      while (true)
      {
        if(r1.get_flag () == false)
        {
          synchronized(r1)
          {
            r1.increment_decrement("Producer",true);
          }
        }
      }
    }

   public void waitForThread () throws InterruptedException
   {
     t1.join ();
   }
}

class consumer implements Runnable
{
   resource r2;
   Thread t2;

   consumer(resource r)
   {
     r2 = r;
     t2 = new Thread (this);
     t2.start();
   }

   public void run()
   {
     while (true)
     {
       if(r2.get_flag () == true)
       {
         synchronized(r2)
         {
           r2.increment_decrement("Consumer",false);
         }
       }
     }
   }

   public void waitForThread () throws InterruptedException
   {
     t2.join ();
   }
} 

public class producercon
{
   public static void main(String args[])
   {
     try
     {
        System.out.print("PRESS CTRL+C TO TERMINATE\n");

        resource r = new resource();
        consumer c = new consumer(r);
        producer p = new producer(r);

        c.waitForThread ();
        p.waitForThread ();
     }
     catch(InterruptedException e)
     {
     }
   }
}

On a totally unrelated note, you really should name your classes with a uppercased first letter : Producer, Consumer, Resource... — Olivier Croisier, Dec 09 '11 at 16:10
@OlivierCroisier: agreed, (a code of a friend of mine, posted the original) — phoxis, Dec 09 '11 at 16:15

score 6 · Accepted Answer · answered Dec 09 '11 at 16:07

6

Your call to get_flag() is not thread safe nor volatile. This means in the cache of thread 1 it can be true while in the cache of thread 2 it can be false.

answered Dec 09 '11 at 16:07

Peter Lawrey

525,659
79
751
1,130

but why does making `get_flag ()` synchronized work ? does making it synchronized enforce the flag to be read from the memory? – phoxis Dec 09 '11 at 16:14
2

Yes. so does making it `volatile`. Otherwise the value can be cached locally or even placed in a register and not read from memory. You can write this code without synchronized at all. ;) – Peter Lawrey Dec 09 '11 at 16:16

score 4 · Answer 2 · answered Dec 09 '11 at 16:08

4

You need to make your boolean either volatile or an AtomicBoolean. Right now multiple threads are trying to access the boolean that is in no way synchronized.

answered Dec 09 '11 at 16:08

Gray

115,027
24
293
354

but why does making `get_flag ()` synchronized work ? does making it synchronized enforce the flag to be read from the memory? – phoxis Dec 09 '11 at 16:14
1

It forces an update or access to be synchronized between threads. With `volatile` this is accomplished with the memory barrier which ensures that all updates by other threads are completed before the value is read on the current thread. `AtomicBoolean` also guarantees that the updates are synchronized between threads. – Gray Dec 09 '11 at 16:16

score 0 · Answer 3 · answered Dec 09 '11 at 16:16

This producer/consumer implementation is quite weird.

Neither the producer not the consumer wait for the resource to be in the adequate state, and the resource access is not well protected (the flag should be guarded by some lock to ensure its visibility between threads).

One way to improve on this design would be to use the standart wait/notify system. Another way would be to use a Semaphore in the Resource to ensure only one thread can access the resource at one given time. Finally, you could use a higher-level construct such an java.util.concurrent.SynchronousQueue to pass some data directly from the producer to the consumer.

yes, that is correct, but the main goal was to know why was the program halting. — phoxis, Dec 09 '11 at 16:41

Why does this threaded code hang

3 Answers3