Help for extended version of Produce/Consumer

[Gman36]

Hi all, I am having a problem with the synchronizing the threads in the classic produce consume thread example. Below is my code for it.

public class Magpie extends Thread {
    private TreeHole treehole;
    private int number;

    public Magpie(TreeHole t, int number) {
        treehole = t;
        this.number = number;
    }

    public void run() {
        int value = 0;
        for (int i = 0; i < 5; i++) {
            value = treehole.get();
            System.out.println("Magpie " + this.number + " got: " + value);
        }
    }
}

class Squirrel extends Thread {
    private TreeHole treehole;
    private int number;

    public Squirrel(TreeHole t, int number) {
        treehole = t;
        this.number = number;
    }

    public void run() {
        for (int i = 0; i < 5; i++) {
            treehole.put(i);
            System.out.println("Squirrel " + this.number + " put: " + i);
            try {
                sleep((int)(Math.random() * 100));
            } catch (InterruptedException e) {
            }
        }
    }
}//end class

//import java.util.concurrent.Executors;
//import java.util.concurrent.ExecutorService;


public class SquirrelMagpieTest {
    public static void main(String[] args) {
        TreeHole t = new TreeHole();
        Squirrel s1 = new Squirrel(t, 1);
        Magpie t1 = new Magpie(t, 1);

		//ExecutorService threadExecutor = Executors.newCachedThreadPool();

		//threadExecutor.execute (t1);
		//threadExecutor.execute (s1);
		
		s1.start();
		t1.start();
       
       
    }
}

class TreeHole 
	{

    private int seq;        
    private boolean available = false;

    public synchronized int get() 
		{
			while (available == false) 
			{
            try {
                wait();
				} catch (InterruptedException e) {
												 }
			}

        available = false;
        notifyAll();
        return seq;
		
		}

    public synchronized void put(int value) 
		{
			while (available == true) 
				{
            try {
                wait();
				} catch (InterruptedException e) {
												 }
			
		}

        seq = value;
        available = true;
        notifyAll();
    }

}

I am having trouble synchronizing the threads and this is evident when sometimes the output will work fine with the magpie taking the int after the squirrel but then sometimes it will take the int before the squirrell has had a chance of putting the new int into the treehole value. I am also looking at extended this to increase N and M number of squirrels and magpies specified by a user, any suggestions on this would also be greatly apprectiated.

Cheers to all.

[JamesCherrill]

Your code looks 100% OK to me. If the magpie gets the right int "before the squirrel has put it", then this implies that things are working OK - otherwize the magpie would just get the previous int twice. Maybe when the magpie gets the int it should reset seq to 0, just to be sure what's happening.
Does anyone know if System.out.println is guaranteed to write lines in the correct order when called from multiple threads?

ps
available == false is redundant for !available
and
available == true is redundant for available

[kvprajapati]

Three corrections:

Thread Magpie and Squirrel requires Object lock and volatile modifier applied to available boolean field of TreeHole.

public class Magpie extends Thread {
    private TreeHole treehole;
    private int number;

    public Magpie(TreeHole t, int number) {
        treehole = t;
        this.number = number;
    }
    public void run() {
        int value = 0;
        for (int i = 0; i < 5; i++) {
       // Object lock
        synchronized(treehole) {
            value = treehole.get();
            System.out.println("Magpie " + this.number + " got: " + value);
      }
        }
    }
}

class Squirrel extends Thread {
    private TreeHole treehole;
    private int number;

    public Squirrel(TreeHole t, int number) {
        treehole = t;
        this.number = number;
    }

    public void run() {
        for (int i = 0; i < 5; i++) {
        //Object lock
         synchronized(treehole) {
            treehole.put(i);
            System.out.println("Squirrel " + this.number + " put: " + i);
            }
            try {
                sleep((int)(Math.random() * 1000));
            } catch (InterruptedException e) {
            }
        }
    }
}

class TreeHole 
	{

    private int seq;        
    private  volatile boolean available = false;

    public synchronized int get() 
		{
			while (available == false) 
			{
            try {
                wait();
				} catch (InterruptedException e) {
												 }
			}

        available = false;
        notifyAll();
        return seq;
		
		}

    public synchronized void put(int value) 
		{
			while (available == true) 
				{
            try {
                                             wait();
				} catch (InterruptedException e) {
												 }
			
		}

        seq = value;
        available = true;
        notifyAll();
    }

}

[JamesCherrill]

Hi adatapost; I'm sorry, but I don't agree this time!
This particular pattern is one where the get and put methods perform all the necessary synchronisation, in a way that can be scaled to longer sequences of operations that need to be serialised. Object locks can achieve the same result in terms of blocking concurrent access, but cannot provide the sequence control that's needed here. In this case they are neither needed nor helpful.
Although volatile is generally useful in code like this, boolean assignments are atomic in Java. Also, in this pattern the boolean assignments are within the scope of the code managed by the wait conditions, so there is no possibility of concurrent modification. Either way, it's not needed in this particular case.
J.

[Gman36]

Thanks for your help so far guys, greatly appreciated. Now I'm trying to increase the number of squirrels and magpies to a int value suggested by the user as stated before. Below is modified version of SquirrelMagpieTest, However I can't figure how to do this properly, The user input ends up just being the number of the squirrel, any direction would be awesome, cheers.

import java.util.Scanner;


public class SquirrelMagpieTest {
    public static void main(String[] args) {
		 
		Scanner input = new Scanner(System.in);
		int number1;
		int number2;

		System.out.print("Enter number of squirrels?");
		number1 = input.nextInt();

		System.out.print("Enter number of magpies?");
		number2 = input.nextInt();
		
		TreeHole t = new TreeHole();
		Squirrel s1 = new Squirrel(t, number1);
		Magpie t1 = new Magpie(t, number2);


		
		s1.start();
		t1.start();
       
       
    }
}

Now I'm not sure i am going around this the right way, I am thinking it should have something to do with TreeHole, because its holding the number of ints and squirrels and magpies are either taking them out or putting them in. I'm not sure what direction I should head in, or what I should do?

[JamesCherrill]

Treehole needs to be enhanced with Collection of some sort to hold the ints that the Squirrels put in. Squirrels add to the Collection, Magpies remove from it. The wait condition for get() needs to test that the Collection is not empty. The wait condition for put(...) needs to test that there is room in the hole (ie that the Collection has not exceeded some arbitrary maximum size).

[kvprajapati]

Thank JC for your valuable suggestion. I have use an array in TreeHole class and two pointers - wpos and rpos. Squirrel write at wpos index and Magpie read at rpos index.

class TreeHole 
	{
    private int []coll=new int[2];
    private int wpos=0,rpos=0,i=0;
    public synchronized void put(int val)  throws Exception {
        if(i==2)
              wait();
        i++;
        coll[wpos]=val;
        wpos++;
        if(wpos>=2)
           wpos=0;
        if(i==1) notify();
     }    
    public synchronized int get() throws Exception {
          if(i==0) wait();
          int val=coll[rpos];
           i--;
          rpos++;
          if(rpos>=2)
              rpos=0;
          if(i==1) notify();
          return val;
    }
}

class Squirrel extends Thread {
    private TreeHole treehole;
    private int number;

    public Squirrel(TreeHole t, int number) {
        treehole = t;
        this.number = number;
    }

    public void run() {
        for (int i = 0; i < 5; i++) {
           try
            {
             treehole.put(i);
            System.out.println("Squirrel " + this.number + " put: " + i);
               sleep((int)(Math.random() * 100));
             } catch (Exception e) {
            }
        }
    }
}

public class Magpie extends Thread {
    private TreeHole treehole;
    private int number;

    public Magpie(TreeHole t, int number) {
        treehole = t;
        this.number = number;
    }

    public void run() {
        int value = 0;
        for (int i = 0; i < 5; i++) {
           try{
              value = treehole.get();
             System.out.println("Magpie " + this.number + " got: " + value);
              }catch(Exception ex) {}
        }
    }
}

public class SquirrelMagpieTest {
    public static void main(String[] args) {
        TreeHole t = new TreeHole();
        Squirrel s1 = new Squirrel(t, 1);
        Magpie t1 = new Magpie(t, 1);
       s1.start();
      t1.start();
    }
}

[JamesCherrill]

Two things: 1. Use while() rather if() because a wait() may be interrupted by other things than the notify() you intended.
2. You have two blocks of code that update the two positions, and i and the array - I would be tempted to put that code (and that code only!) in a synchronized(coll) block to be certain on no partial updates.
(personally, I would use a Vector to hold the things, add to the end, take from the start. no need for any other variables. Vector is synchronized, so no synchronization needed either).

[kvprajapati]

I appreciate your time. Thank JC.