Java MultiThreading task, three smokers and one agent

Question

the task consists of three smokers which are three threads and one class called Agent which has three attributes Tobacco, Lighters and Paper.
Smokers only have one of these items with them, agent class is supposed to put two items randomly on the table and then Smoker is supposed to check if they are missing those two items and pick them up so they can use them for smoking.

I am having an issue where my output is acting bit odd, smokers tend to pick all of the items they are missing at the same time even though i think i put that they are not allowed to do that.

Main class

public class Main {

    public static void main(String[] args) {
        Agent a = new Agent();
        
        Pusac p1 = new Pusac(0,a,"Paper");
        Pusac p2 = new Pusac(1,a,"Tobacco");
        Pusac p3 = new Pusac(2,a,"Lighters");

        p1.start();
        p2.start();
        p3.start();
    }

}

I make one agent using a constructor and I pass it to the constructor of smokers.Each smoker has one item, ID, and agent.

Smoker class

public class Pusac extends Thread {

    int id;
    Agent a;
    String owns;
    
    public Pusac(int id, Agent a, String owns) {
        this.id = id;
        this.a = a;
        this.owns = owns;
    }
    
    @Override
    public void run() {
        while(true) {
        a.putTable();
        if(this.owns.equals("Paper") && a.tobacco == true && a.lighters == true){
                System.out.println("Smoker with ID " + this.id + " took tobacco and lighters.");
                a.smoke();
        } else if(this.owns.equals("Tobacco") && a.lighters == true && a.paper == true) {
                System.out.println("Smoker with ID " + this.id + " took lighters and paper.");
                a.smoke();
        } else if(this.owns.equals("Lighters") && a.paper == true && a.tobacco == true) {
                System.out.println("Smoker with ID " + this.id + " took paper and tobacco.");
                a.smoke();
        }
        }
    }
    
}

In the smoker class function, I check if that agent has items needed for the current thread smoker and then I call the function that is in Agent class.

Agent class

public class Agent {

    boolean paper;
    boolean tobacco;
    boolean lighters;
    
    
    public  void putTable() {
        Random random = new Random();
        int randomNumOne = random.nextInt(3);
        
        switch(randomNumOne) {
        case 0:
            paper = true;
        case 1: 
            tobacco = true;
        case 2:
            lighters = true;
        }
        
        int randomNumTwo = random.nextInt(3);
        
        if(paper!=true && randomNumTwo==0) {
            paper = true;
        } else if(tobacco!=true && randomNumTwo == 1) {
            tobacco = true;
        } else if(lighters != true && randomNumTwo == 2) {
            lighters = true;
        }
    }
    
    public synchronized void smoke() {
        System.out.println("Smoker with ID " + ((Pusac)Thread.currentThread()).id + " is smoking.");
        try {
            Thread.sleep(2000);
        } catch (InterruptedException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
        
        if(((Pusac)Thread.currentThread()).owns.equals("Paper")) {
            this.tobacco = false;
            this.lighters = false;
            System.out.println("Smoker with ID " + ((Pusac)Thread.currentThread()).id + " put down tobacco and lighters.");
        } else if(((Pusac)Thread.currentThread()).owns.equals("Tobacco")) {
            this.paper =false;
            this.lighters =false;
            System.out.println("Smoker with ID " + ((Pusac)Thread.currentThread()).id + " put down paper and lighters.");
        } else if (((Pusac)Thread.currentThread()).owns.equals("Lighters")) {
            this.tobacco =false;
            this.paper =false;
            System.out.println("Smoker with ID " + ((Pusac)Thread.currentThread()).id + " put down paper and tobacco.");
        }
        
  }
}

A final class called Agent consists of a function that randomly puts two items on the table items are boolean and then I have function smoke that just checks which smoker was smoking and then sets everything to be false.

Output I am getting

Smoker with ID 2 took paper and tobacco.
Smoker with ID 1 took lighters and paper.
Smoker with ID 0 took tobacco and lighters.
Smoker with ID 2 is smoking.
Smoker with ID 2 put down paper and tobacco.
Smoker with ID 2 took paper and tobacco.
Smoker with ID 0 is smoking.
Smoker with ID 0 put down tobacco and lighters.
Smoker with ID 0 took tobacco and lighters.
Smoker with ID 1 is smoking.

This is wrong because in reality only one should be picking something at a time and then smoking it so others can have it also.

Thank you for your help sincerely.

score 1 · Accepted Answer · answered Dec 05 '21 at 14:51

1

This is wrong because in reality only one should be picking something at a time and then smoking it so others can have it also.

Because your Pusac Thread class run method is not guarded by anyone, you need to synchronize the run code block using the same monitor which you used to lock for smoke method. You can try synchronizing both smoke and run method by Agent.class

public void smoke() {
    synchronized(Agent.class) {
        ......
    }
}

@Override
public void run() {
    while(true) {
        synchronized(Agent.class) {
            ......
        }
    }
}

answered Dec 05 '21 at 14:51

Turtle

667
1
6
18

Quick question before i try to synchronize run() method, didn't i synchronize smoke() method by saying ```public synchronized void smoke()``` shouldn't that secure that the method smoke is used only by one thread at a time? – CupidONO Dec 05 '21 at 15:16
Also i just synchronized by Agent.class but that tends to lock it only for one thread.Here is the current output ```Smoker with ID 1 took lighters and paper. Smoker with ID 1 is smoking. Smoker with ID 1 put down paper and lighters. Smoker with ID 1 took lighters and paper. Smoker with ID 1 is smoking.``` To be more specific i surrounded my if statements in run method wtih ```synchronized(Agent.class)``` – CupidONO Dec 05 '21 at 15:24
@CupidONO only `smoke` method is guarded by your code. Inside the `run` method more than two threads can enter at the same time. – Turtle Dec 05 '21 at 15:25
aha i understand, but when i surround my if statements in run method.I tend to get that weird output where my Agent class generates same things each time and only one thread picks them up.The output i provided in reply above. – CupidONO Dec 05 '21 at 15:28
@CupidONO I could get different threads in the output when i executed. op : `Smoker with ID 0 took tobacco and lighters. Smoker with ID 0 is smoking. Smoker with ID 0 put down tobacco and lighters. Smoker with ID 2 took paper and tobacco. Smoker with ID 2 is smoking. Smoker with ID 2 put down paper and tobacco.` – Turtle Dec 05 '21 at 15:37
Is there a way you could provide me the changes you made on my code.I might have changed something incorrectly.Thank you in forward – CupidONO Dec 05 '21 at 15:41
@CupidONO try adding `Thread.sleep(100);` inside the run method before `synchronized` block so that other threads can acquire the lock in this time and you can see the change in output. – Turtle Dec 05 '21 at 15:44
It actually worked this time, could you please explain to me why it happens to work now after i added Thread.sleep() Do threads fight over the resources?Also would i need to have it synchronized inside the run if i only was calling a.smoke() once for all threads without special if statements. – CupidONO Dec 05 '21 at 15:48
@CupidONO Yes you would need to `synchronize` even if you remove the `if` conditions in case if you want to prevent multiple threads from entering the block at the same time. Regarding `sleep` previously since while loop is continuous there is a high chance that the same thread will reacquire the lock and execute, and increasing sleep will cause current thread to sleep and give other threads the chance to acquire lock. Although this whole mechanism is taken care by Thread scheduler. – Turtle Dec 05 '21 at 15:53
So what synchronized(Name.class) does is basically locks that entire class for that one thread so that others can't access it at that given moment right? So is synchronized on void smoke() really necessary if that is the case with synchronized(Agent.class) – CupidONO Dec 05 '21 at 15:56
@CupidONO Yes right, [this](https://stackoverflow.com/questions/2056243/java-synchronized-block-for-class) is a good reference to look into – Turtle Dec 05 '21 at 16:00

score 1 · Answer 2 · edited Dec 13 '21 at 18:15

When sharing mutable variables between threads those variables must be handled differently as otherwise each of the reading threads might see different values.

tldr:

Simplest way to fix your existing program is most likely to define all of the variables "volatile", like

volatile boolean paper;
volatile boolean tobacco;
volatile boolean lighters;

The somewhat longer explanation: for performance reasons threads may read variables not via (slow) RAM access, but fetch them from cache. This works as long as only the same thread reads and writes the variable, but fails when you try to set the value in one thread and read it back from a different one. The reading thread may see still the older value.

One way to fix it is declare the variable volatile. This basically instructs the value to always be read from RAM - which should not make much of a difference for a project of this size.

You could also use java.util.concurrent.atomic.AtomicBoolean instead of a primitive boolean, which should result in the same thing for this use-case.

Another way to fix the issue is to ensure all write changes on the variable are within a synchronized-block. This way the overhead is on the write-process and no longer on the readers.

However, as thread safety becomes a deep topic fast, if you are planning to do more multithreaded applications like this there is some extensive reading material on the topic out there ;-)

Thank you so much for the heads up, i did not put volatile and i totally forgot about it.Though it did not fix the issue it really reminded me how imporatant volatile is in Threads and that i should be more focused to use them.Thank you very much, also i love Threads and yeah i am kind of doing this for university only but i will also do some research of my own afterwards.Thank you again ^_^ — CupidONO, Dec 05 '21 at 15:17
@Roland: caches on modern CPU's are always coherent and they are write-behind. So changes to the cache do not need to be propagated to main memory and CPUs will not see incoherent data. So the mental model of writing to main memory, even though very common, is incorrect. For more info see https://shipilev.net/blog/2016/close-encounters-of-jmm-kind/#myth-reorderings-and-commit — pveentjer, Dec 06 '21 at 07:58

Java MultiThreading task, three smokers and one agent

Main class

Smoker class

Agent class

Output I am getting

2 Answers2