-2

I have this threaded program that has three threads: main, lessonThread, questionThread.

It works like this:

  • Lesson continues continues to gets printed while the finished the variable is true;
  • every 5 seconds the questionThread asks Finish the lesson? and if the answer is y, it sets finished to false

The problem is that the Lesson continues never gets printed after the question gets asked the first time:

enter image description here

Also, as seen on the picture, sometimes lessonThread sneaks in with its Lesson continues before the user can enter the answer to the questionThread's question.

public class Lesson {
    private boolean finished;
    private boolean waitingForAnswer;
    private Scanner scanner = new Scanner(System.in);

    private Thread lessonThread;
    private Thread questionThread;

    public static void main(String[] args) {
        Lesson lesson = new Lesson();
        lesson.lessonThread = lesson.new LessonThread();
        lesson.questionThread = lesson.new QuestionThread();

        lesson.lessonThread.start();
        lesson.questionThread.start();
    }



    class LessonThread extends Thread  {
        @Override
        public void run()  {
            while (!finished && !waitingForAnswer)  {
                System.out.println("Lesson continues");
            }
        }
    }

    class QuestionThread extends Thread {
        private Instant sleepStart = Instant.now();
        private boolean isAsleep = true;
        @Override
        public void run() {
            while (!finished) {
                if (isAsleep && Instant.now().getEpochSecond() - sleepStart.getEpochSecond() >= 5) {
                    System.out.print("Finish a lesson? y/n");
                    waitingForAnswer = true;
                    String reply = scanner.nextLine().substring(0, 1);
                    switch (reply.toLowerCase()) {
                        case "y":
                            finished = true;
                    }
                    waitingForAnswer = false;

                    isAsleep = true;
                    sleepStart = Instant.now();
                }
            }
        }
    }
}

I think the waitingForAnswer = true might be at fault here, but then, the lessonThread has 5 seconds until the questionThread asks the question again, during which the waitingForAnswer is false.

Any help is greatly appreciated.

EDIT: I found a buy in the loop in the lessonThread and changed it to:

    @Override
        public void run()  {
            while (!finished)  {
                if (!waitingForAnswer)  {
                    System.out.println("Lesson continues");
                }
            }
        }

However, I get the same result.

EDIT: I can get it working when inside a debugger:

enter image description here

Akshay Ashok
  • 107
  • 2
  • 9
parsecer
  • 4,758
  • 13
  • 71
  • 140
  • Does this answer your question? [java threads don't see shared boolean changes](https://stackoverflow.com/questions/17385211/java-threads-dont-see-shared-boolean-changes) – pafau k. May 31 '20 at 22:34
  • @pafauk. No. My `boolean finished` and `boolean waitingForAnswer` are **not** local - they are inside a wrapper class that is visible to both threads – parsecer May 31 '20 at 22:37
  • @parsecer The answer there talks about a "**thread** local" variable. While this term is questionable, the essence is correct (at least without nitpicking about the JMM). Also see https://stackoverflow.com/questions/106591/ – Marco13 May 31 '20 at 22:45
  • 1
    finished and waitingForAnswer need to be made volatile. – NomadMaker May 31 '20 at 23:04
  • Your question say *continues ... while the `finished` variable is `true`*. Logically, processing should continue while the state is "not finished", so your code should continue while `finished` variable is `false`. ie a variable named `finished` should mean "finished" – Bohemian May 31 '20 at 23:17
  • @Bohemian that's exactly how it works... `finished == false ? lesson continues : exit program` – parsecer May 31 '20 at 23:34
  • `while (!finished) {` – parsecer May 31 '20 at 23:34
  • 1
    @parsecer but that’s not what your question says; the 2 points after “it works like this” say that exact opposite of that – Bohemian May 31 '20 at 23:59

2 Answers2

4

this just isn't how you're supposed to work with threads. You have 2 major problems here:

  1. java memory model.

Imagine that one thread writes to some variable, and a fraction of a second later, another thread reads it. If that would be guaranteed to work the way you want it to, that means that write has to propagate all the way through any place that could ever see it before code can continue.. and because you have absolutely no idea which fields are read by some thread until a thread actually reads it (java is not in the business of attempting to look ahead and predict what the code will be doing later), that means every single last write to any variable needs a full propagate sync across all threads that can see it... which is all of them! Modern CPUs have multiple cores and each core has their own cache, and if we apply that rule (all changes must be visible immediately everywhere) you might as well take all that cache and chuck it in the garbage because you wouldn't be able to use it.

If it worked like that - java would be slower than molasses.

So java does not work like that. Any thread is free to make a copy of any field or not, at its discretion. If thread A writes 'true' to some instance's variable, and thread B reads that boolean from the exact same instance many seconds later, java is entirely free to act as if the value is 'false'... even if when code in thread A looks at it, it sees 'true'. At some arbitrary later point the values will sync up. It may take a long time, no guarantees are available to you.

So how do you work with threads in java?

The JMM (Java Memory Model) works by describing so called comes-before/comes-after relationships: Only if code is written to clearly indicate that you intend for some event in thread A to clearly come before some other event in thread B, then java will guarantee that any effects performed in thread A and visible there will also be visible in thread B once B's event (the one that 'came after') has finished.

For example, if thread A does:

synchronized (someRef) {
    someRef.intVal1 = 1;
    someRef.intVal2 = 2;
}

and thread B does:

synchronized(someRef) {
    System.out.println(someRef.intVal1 + someRef.intVal2);
}

then you are guaranteed to witness in B either 0 (which will be the case where B 'won' the fight and got to the synchronized statement first), or 3, which is always printed if B got there last; that synchronized block is establishing a CBCA relationship: The 'winning' thread's closing } 'comes before' the losing thread's opening one, as far as execution is concerned, therefore any writes done by thread A will be visible by thread B by the time it enters it sync block.

Your code does not establish any such relationships, therefore, you have no guarantees.

You establish them with writes/reads from volatile fields, with synchronized(), and with any code that itself uses these, which is a lot of code: Most classes in the java.util.concurrent package, starting threads, and many other things do some sync/volatile access internally.

  1. The flying laptop issue.

It's not the 1980s anymore. Your CPU is capable of doing enough calculations at any given moment to draw enough power to heat a small house comfortably. The reason your laptop or desktop or phone isn't a burning ball of lava is because the CPU is almost always doing entirely nothing whatsoever, and thus not drawing any current and heating up. In fact, once a CPU gets going, it will very very quickly overheat itself and throttle down and run slower. That's because 95%+ of common PC workloads involve a 'burst' of calculations to be done, which the CPU can do in a fraction of a second at full turboboosted power, and then it can go back to idling again whilst the fans and the cooling paste and the heat fins dissipate the heat that this burst of power caused. That's why if you try to do something that causes the CPU to be engaged for a long time, such as encoding video, it seems to go a little faster at first before it slows down to a stable level.. whilst your battery is almost visibly draining down and your fans sound like the laptop is about to take off for higher orbit and follow Doug and Bob to the ISS - because that stable level is 'as fast as the fans and heat sinks can draw the heat away from the CPU so that it doesn't explode'. Which is not as fast as when it was still colder, but still pretty fast. Especially if you have powerful fans.

The upshot of all this?

You must idle that CPU.

something like:

while (true) {}

is a so-called 'busy loop': It does nothing, looping forever, whilst keeping the CPU occupied, burning a hole into the laptop and causing the fans to go ape. This is not a good thing. If you want execution to wait for some event before continuing, then wait for it. Keyword: wait. If you just want to wait for 5 seconds, Thread.sleep(5000) is what you want. Not a busy-loop. If you want to wait until some other thread has performed a job, use the core wait/notifyAll system (these are methods on j.l.Object and interact with the synchronized keyword), or better yet, use a latch or a lock object from java.util.concurrent, those classes are fantastic. If you just want to ensure that 2 threads don't conflict while they touch the same data, use synchronized. All these features will let the CPU idle down. endlessly spinning away in a while loop, checking an if clause - that is a bad idea.

And you get CBCA relationships to boot, which is what is required for any 2 threads to communicate with each other.

And because you're overloading the CPU with work, that sync point where your '= false' writes get synced back over to the other thread probably aren't happening - normally it's relatively hard to observe JMM issues (which is what makes multithreaded programming so tricky - it is complex, you will mess up, it's hard to test for errors, and it's plausible you'll never personally run into this problem today. But tomorrow, with another song on the winamp, on another system, happens all the time). This is a fine way to observe it a lot.

rzwitserloot
  • 85,357
  • 5
  • 51
  • 72
  • Thank you a lot for the detailed answer! I understand now that the caching was the problem and that there are so many underwater obstacles in JVM/JMM implementation that it's better to follow the rules and use the designed locks/synchronized blocks/etc when using threads. Why is the `volatile` keyword so bad though? Because it would case the performance drop due to cache updates? – parsecer Jun 01 '20 at 17:13
  • 1
    volatile is more or less fine (but finding something suitable in the j.u.c package is usually better), but never sleeping the thread is the bad thing. busy waiting on a volatile variable, that's the bad part. – rzwitserloot Jun 01 '20 at 23:00
-1

I managed to make it work with making waitingForAnswer volatile:

    private volatile boolean waitingForAnswer;
parsecer
  • 4,758
  • 13
  • 71
  • 140
  • and your code murders CPUs even with this fix. See my answer for how you're supposed to work with threads in java and an explanation for why 'volatile' appears to fix it (but not in a good way - see my answer for good ways to fix this). – rzwitserloot May 31 '20 at 23:51