I'm studying multithreading and wrote a simple program that simulates a classroom. There are three main classes: WhiteBoard, Student and Teacher.
The WhiteBoard is a singleton that holds a string containing text written by the teacher, and also the methods to write and read from that string. The teacher object will write on the WhiteBoard and wait for all the students to read. When all students are done, they will notify the teacher to write again. The program ends when he writes "end".
When i tested the code, i noticed that some students read the text more than once. Here's a screenshot with the output. As you can see, Pablo read the text twice in a row. How is that possible? I thought that when we call the method notify( ), one of the threads waiting in the monitor's queue would be woken up. Does that include the calling thread itself? If not, why this is happening and how to fix this correctly?
Thanks for any help! Here is the code:
class WhiteBoard {
// Text written in the whiteboard
private String texto;
// Total number of students
private int noOfStudents = 0;
// Counter that keep control of which thread is accessing the whiteboard
private int counter = 0;
// Whiteboard instance
private static WhiteBoard wbInstance;
// Private constructor
private WhiteBoard() { }
// Returns the instance of the whiteboard (i'm using a singleton pattern)
public static WhiteBoard getInstance() {
if(wbInstance == null)
wbInstance = new WhiteBoard();
return wbInstance;
}
// This method is called when a new student is created
public void oneMoreStudent() {
noOfStudents ;
}
public synchronized void write(String str) {
// If it's not the teacher's turn to write, his thread waits
while(counter != 0) {
try {
wait();
}
catch(InterruptedException e) {
e.printStackTrace();
}
}
// Write's the text
texto = str;
// Increases the counter
counter ;
// Notify students threads
notifyAll();
}
public synchronized String read() {
// If it's not the students' turn to read, their threads wait
while(counter == 0) {
try {
wait();
}
catch(InterruptedException e) {
e.printStackTrace();
}
}
// Saves current text
String temp = texto;
if(counter == noOfStudents)
{
// If everyone read, reset counter and notify the teacher
counter = 0;
notify();
}
else
// If not, increase counter
counter ;
// Return the text
return temp;
}
}
// Student class
class Student implements Runnable {
// Student name
private String name;
// Whiteboard instance
private WhiteBoard wb;
// Constructor
public Student(String nome)
{
this.name = nome;
wb = WhiteBoard.getInstance();
wb.oneMoreStudent();
}
@Override
public void run() {
String txtCopiado;
while (true) {
// Reads the text
txtCopiado = wb.read();
// If the text is "end", breaks the while loop
if(txtCopiado.equals("end"))
break;
// Print student name and the text he read
System.out.println(name ": " txtCopiado);
}
}
}
// Teacher class
class Teacher implements Runnable {
// This is the array of texts he will write
private String[] textos = {"Ola amigos", "Java Programming", "Jacare", "end"};
// Whiteboard instance
private WhiteBoard wb;
// Constructor
public Teacher() {
wb = WhiteBoard.getInstance();
}
@Override
public void run() {
// Write each text on the whiteboard
for(String str : textos) {
wb.write(str);
// Print what the teacher wrote
System.out.println("Teacher: " str);
}
}
}
// Main class
public class Classroom {
public static void main(String[] args) {
Thread prof = new Thread(new Teacher());
Thread alun1 = new Thread(new Student("1. Pablo"));
Thread alun2 = new Thread(new Student("2. Kiki"));
Thread alun3 = new Thread(new Student("3. Lucas"));
prof.start();
alun1.start();
alun2.start();
alun3.start();
}
}
CodePudding user response：
There are lots of little things wrong with this code; wrong in ways that you might not ever be able to reproduce, because the Java Memory Model, which is rather relevant when dealing with multi-threaded code written with these obsolete/ill-advised strategies, is filled to the brink with 'may' - a JVM might do one thing or it might do another. This makes testing impossible (maybe on your hardware, on this day, with this song on your music player, it always goes one way. Tomorrow it might go a different way).
You may be thinking: What the heck, that sounds downright stupid, why would java be designed to act like an evil monkey's pay?? This answer cannot be right!
Regrettably, it really works like that. The thing is, trying to give you a ton of guarantees (such as: All things ran by a JVM always act like they ran in some order), then any realistic java app would run incredibly slowly - giving such guarantees is incredibly difficult, given that modern CPUs ('modern' - 20 years at this point) have multiple cores and each core has multiple pipelines, and CPUs cannot access main memory at all (they can only access on-die caches, and they can ask the memory controller to swap one cached page out for another, they can't ask the memory controller to read or write a single value and CPUs haven't been able to do that for decades now). Because JVMs are supposed to run on many platforms, there are really only 2 options:
- The java spec gives all sorts of guarantees, and thus JVMs are slow.
- A spec (The JMM -Java Memory Model) spells out the many 'rights' as JVM enjoys about what various weird things it gets to do, and as a consequence many JVM impls exist that do quite different things, because they all need to do what 'works fast' on the local architecture and OS. This spec tells you the few things that are guaranteed, and as a programmer your job is to stay within these guarantees. Knowing that colouring outside the lines is a problem that on your hardware you may never be able to test for, ouch.
As you might surmise, #2 is the actual way java works.
This more generally highlights why you shouldn't do any of this. Using wait() and notify() requires a very high level of expertise because it is effectively impossible to test. It's very hard to get this expertise: Ordinarily, you learn best by trying stuff, but here, trying stuff will just mislead you: You end up writing some code that seems to work reliably only to realize much later on that it doesn't work. It's just that at the time you wrote it, for whatever reason, the 'the JVM may do X or Y' aspect of it always went the same way. Or, on a certain CPU it always goes one way, but on another CPU that isn't always true.
I'll name a few:
noOfStudents violated HB/HA. (I'll explain what HB/HA is in a bit).
Your student1 thread starts and holds no locks (Student's run() method isn't synchronized, for example), then invokes oneMoreStudent() on the one Whiteboard instance. This then runs noOfStudents . noOfStudents is an ordinary, non-volatile field.
This is a JMM violation. The JMM states that a JVM implementation is allowed to re-order statements (run stuff in any order), and to locally cache any field of any object at any time (as in, give each thread its own little copy of that noOfStudents field), or not, at will. That means each student thread could be incrementing its local cached copy of noOfStudents, (so each student thread would see that noOfStudents is 1, even after all 3 have ran and it is clear, from other evidence, they've all long since passed that oneMoreStudent() call - remember, the JVM is free to re-order or to appear re-ordered if you don't establish HBHA). And Teacher sees 0, even. Which messes up its loop and would explain that your code doesn't work. It isn't necessarily the right explanation or the only way, just, it's one of a few problems that could cause what you witness, depending on your CPU, your OS, and the phase of the moon.
HB/HA
Happens-Before / Happens-After. As I said, the JVM is free to re-order statements and to create local caches with undefined synchronization behaviour (any local cache can overwrite any other, or not, at any time, with no guarantees whatsoever) -- unless HB/HA is involved.
HB/HA is a set of rules for which the JMM guarantees that a 'happens-before' / 'happens-after' relationship exists. One action (e.g. a statement) is 'HB' relative to some other action (another statement).
In which case, the JVM guarantees that, other than timing (how long things take), it is not possible to observe from the HA line any state such as it was before the HB line ran. In other words, a "Happens After" line is guaranteed to happen after the 'Happens Before' line it is HA to, unless it wouldn't be possible for you to observe this except using very sly trickery such as using timing, in which case the JVM is free to ignore HB/HA rules.
One trivial way to gain HB/HA relationships is consecutive statements executing in the same thread: Given void foo() { a(); b(); } That b() is HA relative to a() for the obvious reason.
Another way is threads itself: thread.start() is HB relative to the first line in the run() method of the Runnable that thread ends up starting. Similarly, thread.yield()'s returning is HA relative to the last statement that thread ran.
The common ways to establish HB/HA is synchronized: If 2 threads are both executing code that lock the same monitor (synchronized(x) where x is referring to the exact same object in both threads), one 'wins' and runs first - all synchronized blocks are 'ordered' in some undefined order. Whatever order that might be, the entering and exiting is all HB/HA to each other as you'd expect - if one thread 'goes first', then its breaking out of the synchronized is HA relative to the other thread's entering of it.
Because HB/HA guarantees you can't witness things running out of order, you solve the caching issue - the JMM guarantees that it is no longer the case that each thread could be operating on a cached value.
Making oneMoreStudent synchronized therefore solves this particular problem: One thread (at random, more or less, there is no guarantee that if you start 3 threads in a row, that the first started thread also starts running first) wins and passes the synchronized {} first, updates that value, exits the synchronized block, and in so doing is HB relative to the next thread that 'wins' and enters the block, and thus, that cannot observe the state as it was before the first thread exited: noOfStudents having been incremented is a change in state, thus, the second thread is guaranteed to see that noOfStudents is 1, and increments it to 2. For the same reason, the third student thread ends up witnessing 2 and increments to 3, and the teacher thread is guaranteed to see it be '3'. Unless, of course.
Violation: Depending on starting order
If the Teacher thread gets truckin' before all 3 student threads have initialized (Such as incrementing noOfStudents), then all heck breaks loose, as the teacher thread will not be waiting for the requisite amount of student reads and will thus write some more before all students managed to read the data. The fact that you start the teacher thread 4th is immaterial, the JVM does not guarantee that threads actually run in order. That'd be silly, the point of threads is to do things simultaneously if at all possible. So, all 4 threads simulatenously get to a point where they need to acquire the lock on the whiteboard object (the synchronized keyword does that), and thus one wins at random. As is usual with the JMM, you can't "prove" this with a test. On your hardware/Java/Phase-of-the-moon setup it may well be that you reliably observe the student threads going first, then the teacher thread. But that is no guarantee, and any JVM that doesn't do that, isn't broken.
To make life even more complicated, System.out is a very big iron call (takes a ton of time) and acquires various locks, which means attempting to sysout your way into understanding what happens severely changes how your app runs, so you can't do that either. Not that witnessing anything is all that useful - JVMs don't have to be consistent when it comes to the JMM.
Singleton violation
Your getInstance() method is itself a violation - your code checks if the whiteboard field is null and if so, makes a new whiteboard. This is on its face already completely wrong: What if 2 threads end up simultaneously invoking getInstance()? Both will check "is it null?" and will then both say: Yeah, it is, they then both make a whiteboard instance, both assign it to the one static whiteboard variable, which may or may not be a local cache. You now have 2 whiteboard instances and obviously the rest of the code will completely fail.
It gets worse, due to the reordering rules, trying to dance around this is more complicated than you think. Search the web for 'singleton double checked locking' for more, but note that 90% of the tutorials out there are dead wrong.
The solution to this is to realize that getInstance() is a silly idea. Java doesn't load all classes on bootup, it loads the main class, and just starts running it. If, as part of running it, some other as-yet-unloaded class comes up, that class is loaded on demand. However, once a class is loaded it stays loaded (unless you are engaging in class loader shenanigans which is beyond the scope of this answer). Java itself therefore needs to do advanced singleton locking because the JMM guarantees that a class is only loaded at most one time. That means this is the only correct way to do a singleton:
class Whiteboard {
private static final INSTANCE = new Whiteboard();
public static Whiteboard getInstance() {
return INSTANCE;
}
}
This works because the JMM guarantees a class is only loaded once, thus guaranteeing that INSTANCE is only assigned to once. THis isn't 'needlessly wasteful' because the class isn't loaded at all unless you actually mention Whiteboard somewhere, in which case... you're going to want that instance. Java's baked in locking behaviour to ensure classes are loaded once is the most efficient locking construct in java available to you.
In the exceedingly, one-in-a-million unlikely chance that you are likely to refer to a class from time to time but not fetch the singleton, and you really want it to lazy-load, this is the correct way to do that:
class Whiteboard {
private static class WhiteboardInstanceHolder {
static final Whiteboard INSTANCE = new Whiteboard();
}
public static Whiteboard getInstance() {
return WhiteboardInstanceHolder.INSTANCE;
}
}
This works because, as I said, java does not load classes until it is actually needed, and here it won't be needed until someone invokes getInstance(). This snippet is much more complicated and the only advantage is that you can mention Whiteboard as a type without invoking .getInstance() and avoid the load. Which generally isn't relevant, so you shouldn't bother with this generally needlessly convoluted example.
Closing notes - what should you really be doing
Any of the above can explain what you witness. And I wouldn't be surprised in the code you wrote. If you fix all 3 and the code works reliably, you really have no idea if you actually fixed it all, or if the errors that remain are simply of a nature that doesn't occur on your hardware/OS combo.
Hence, just.. this is rocket science. You shouldn't be doing this. There are different programming models that are vastly superior and much easier to reason about:
Concurrency 'in the large' - send all inter-thread comms through a medium that is explicitly designed for it. Such as a database (set up a transaction, voila), or a message queue.
With something from the
java.util.concurrentpackage. Here aCyclicBarriercould work, or perhaps one of the many collections it has.