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 constructor and i pass it to constructor of smokers.Each smoker has one item, ID and their 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 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.");
}
}
}
Final class called Agent consists of 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.
CodePudding user response：
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) {
......
}
}
}
CodePudding user response：
When sharing mutable variables between threads those variables must be handled differently as otherwise each of the reading threads might see differnt values.
tldr: simlest way to fix your existing programm is most likely to define all of the variables "volatile", like
volatile boolean paper;
volatile boolean tobacco;
volatile boolean lighters;
The somewhat longer explaination: 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 threadsafety 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 ;-)