Thread runs infinitely because `synchronized` does sync access to a field and 4 methods in a concurrency test with two threads

Question

Things to Note:

• I use JetBrain IntelliJ IDEA Community Edition.

• My code relies on 3 short classes. Easy to read.

• The original code runs as it is and produces the appropriate result. See image at end of post.

EXPTECTED RESULT: First thread1 should update totalBytes , and then thread2 should print the result.

ACTUAL RESULT: thread1 finishes but thread2 runs infinitely, and without reading the update to totalBytes.

ISSUES:

1. The incrementTotalBytes() method does not increment the TotalBytes() field in the “for loop” within the DownloadFileTask class which is invoked by thread1.

2. The synchronized keyword does not synchronize access to the isDone field so that thread2 can run and exit successfully.

To recap: the totalBytes field is not updated, and thread2 does not exits.

Here is class DownloadStatus:

public class DownloadStatus {


private boolean isDone;
private int totalBytes;
private int totalFiles;
private final Object totalBytesLock = new Object();
private  final Object totalFilesLock = new Object();

public void incrementTotalBytes(){
    synchronized (totalBytesLock){ 
        totalBytes++;}}

public void incrementTotalFiles(){
    synchronized (totalFilesLock){ /
        totalFiles++;}}

public int getTotalBytes(){
    return totalBytes;}

public int getTotalFiles(){
    return totalFiles;}

public synchronized boolean isDone(){
   return isDone;}

public synchronized void done(){
    isDone = true;}

}

Here is class DownloadFileTask:

public class DownloadFileTask implements Runnable {
private DownloadStatus status;
public DownloadFileTask(DownloadStatus status) {
    this.status = new DownloadStatus();}

    @Override
    public void run () {
        System.out.println("Downloading a File: " + Thread.currentThread().getName());
        for (var i = 0; i < 1_000_000; i++) {
            if (Thread.currentThread().isInterrupted()) return;
            status.incrementTotalBytes();}

        status.done();
        

        System.out.println();
        System.out.println("Download Complete: " + Thread.currentThread().getName());}

}// End of DownloadFileTask

Here is class ThreadDemo , where I ultimately run my main():

public class ThreadDemo {

public static void show() {
    var status = new DownloadStatus();
    var thread1 = new Thread(new DownloadFileTask(status));
    var thread2 = new Thread(() -> {
        System.out.println("Thread: " + Thread.currentThread().getName() + " has started.");
        while (!status.isDone()) {}
        System.out.println(Thread.currentThread().getName() + " has finished. Total Byes: "+ status.getTotalBytes());
    });

    thread1.start();
    thread2.start();
}

public static void main(String[] args){
            show();
}

}// End of ThreadDemo

WHAT I TRIED WITH NO SUCCESS:

1. Matched the SDK to the JDK used in the video lesson. I Switched from OpenJDK-17.0 to OpenJDK-12.01, and used the same Language Level as JDK of-course.

2. Matched every literal and the syntactic structure of the original code. Total copy.

3. Added an instance of the status.isDone(); thinking that perhaps the done() method was not updating the value of the boolean field isDone.

4. Instead of using a synchronized block, I synchronized every method, incrementTotalBytes(), incrementTotalFiles(), isDone(), and done(). However, I did not synchronize the getTotalBytes() field because this is class member that is updated by thread1 and then thread2 simply reads the total and prints it, first checking while(!status.isDone()) {} .

Here is a screen-capture of the code running. You will notice that thread1 starts, thread2 starts, thread1 completes, thread2 runs infinitely.

ThreadDemo code running

After careful analysis, I am left with these questions:

1. When does thread1 send the interrupt indication for the code in the DownloadFileTask class to validate if (Thread.currentThread().isInterrupted()) return; in the overwritten run(); method?

2. If isDone = true, when does while (!status.isDone()){} become false? There are no methods to change the value of isDone.

This is what is should result into, as in the original code, keep in mind that my code is an exact copy of the lesson code.

RESULT OF ORIGINAL CODE.

Answer 1

There are a number of significant errors in your code. The major problem with this kind of low-level threading code is untestability. Things can seem to run fine but your code is broken: On a different day, a different architecture, a different song playing on your music player, a different phase of the moon - whatever - your code will fail. It's not random as in 'coinflip' - you can't 'fix' this by just rerunning your test a few thousand times. It might run the same way every time today. And then tomorrow it'll run a different way.

This also means you can't just 'play around'. For most APIs and all that sort of thing it's a great idea to play around and see what happens. Not so much with threading.

Too many locks

A deadlock is a very simple concept.

One thread does:

synchronized (objA) {
  synchronized (objB) {
    hi();
  }
}

and another does:

synchronized (objB) {
  synchronized (objA) {
    hello();
  }
}

The problem occurs when thread 1 ends up acquiring the lock on objA, and simultaneously thread 2 ends up acquiring the lock on objB.

You are now deadlocked. Both threads continue to hold on to their lock, and are waiting for the other's lock. It'll never fix itself. At best, java will detect it and tell you, but it can't 'fix' it by unsticking a thread. Because perhaps one thread already ran a few things and a JVM can't just go in there and mess with things like that.

There is one exceedingly trivial way to avoid deadlocks: Just have a single lock.

Your code has 3 locks in a very small snippet! It locks on this (the done() and isDone() methods do that), it locks on totalFilesLock, and it locks on totalBytesLock.

Another problem with threading is that it's difficult to state rules that have literally zero exceptions to them. Hence, take it with the usual 'if you are really experienced and know threading intimately, you may be capable of determining when some piece of advice shouldn't be applied. However, until then...' caveat.

So, having said that: Do not ever have more than one lock. Threading is hard enough. Introducing a cavalcade of locks is most definitely not going to help make things any easier.

Failure to understand the memory model

The JVM operates on an evil coin. Whenever you read a field or write a field - literally any field, so a JVM will be doing that a lot, the evil coin is flipped. On heads, the fieldwrite/fieldread is from main memory. On tails, it's from a cache local to the thread.

The coin is evil in the sense that it can flip heads all day today. And this week. But it lands tails next week when presenting the feature to the boss.

Specifically, the JVM reserves the right (and uses this right a lot) to give each thread a local cache of every field. Any write it does? It just goes to the local cache. Any read it does? From the local cache.

Hence, you can have:

public class Test {
  int x;

  public static void main(String[] args) {
    Test t = new Test();
    Thread a = new Thread(() -> { .. do stuff with t.x .. });
    Thread b = new Thread(() -> { .. do stuff with t.x .. });
    Thread c = new Thread(() -> { .. do stuff with t.x .. });
    a.start();
    b.start();
    c.start();
  }
}

And heave each thread (a, b, and c) be oblivious to the others. They each have their own version of x. It's not a matter of timing. thread a can execute t.x = 100; and thread B can execute while (true) { Thread.sleep(1000); System.out.println(t.x); }, you can leave it running for 3 days, and have thread B prints '0' for the entire 3 day period, with thread a never actually conveying that t.x = 100 to thread B. A JVM isn't obligated to that. It may do that. And this really is how JVMs work. They will actually do that. Sometimes.

The way to avoid it is so-called Happens-Before relationships. That's official jargon straight from the Java Memory Model section of the java virtual machine specification.

HB relationships work, roughly, as follows:

1. The JVM is free to re-order anything. The JVM just throws a dart and runs whatever line it wants in whatever order it wants.
2. However, if there is a relationship between 2 lines of code such that line A is 'HB' relative to line B, then it is not possible to observe at B a state as it was before A, other than via timing. That doesn't quite mean that A is guaranteed to execute before B - if B doesn't do any observing of state that A modifies, the JVM is still free to run them in whatever order you want.
3. There is a specific enumerated list of acts that establish HB.

For example, trivially, in a single thread, lexical order establishes HB. Given int b = 5; b = 10; System.out.println(b); it is not possible for that println to observe 5. Another trivial one is t.start() - the first line in a thread's run() is HA relative to t.start().

synchronized is one simple way to establish HB. Exiting a sync block is HB relative to another thread entering one later. That means all memory writes are now reliably 'synced'.

Hence, your code is broken: The getTotalBytes() method doesn't synchronize on anything, which means there's no HB/HA between incrementTotalBytes and getTotalBytes. You can call increment right now, and have another thread call getTotalBytes literally 5 hours from now and.. have it not see the update.

Misuse of interrupt

You're looking for if (Thread.interrupted()) return. Not if (Thread.currentThread().isInterrupted()) return;

an interrupt doesn't do anything except raise a flag. The JVM takes ZERO action stopping that thread. This flag stays up until something checks it - Thread.interrupted() checks it. You are supposed to lower it once you act. The reason you do that, is that a thread whose interrupt flag is raised is going to fail really soon: Every method in the JDK docs that is specced to throws InterruptedException will instantly throw that if you call them when that flag is up (e.g. Thread.sleep(1000L) is equivalent to throw new InterruptedException() if the flag is up - it waits 0 milliseconds and throws immediately. If that exception is thrown, the flag is always lowered (as the interrupt event has now been handled). However, until you call one of those methods, nothing happens. If you're going to if-check the interrupted flag, the right way is with Thread.interrupted() - you want that flag down if you respond to it.

Note that interrupts do not occur unless you literally stick t.interrupt() somewhere in your code. Force-killing a java app does not cause interrupts. Hitting CTRL+C does not cause interrupts. sending it a hangup signup does not cause interrupts. nothing causes interrupts other than t.interrupt() - which the core java libraries do not ever call, nor do any libraries.

Hence, you should not code any interrupt logic at all unless you intend to use it.

A warning

System.out.println internally synchronized on all sorts of things. Hence, if you litter sysout statements throughout the code to attempt to see all this weirdness, you will never see it - because your code is practically single threaded now due to all the locking System.out does. It is in fact really difficult to casually observe the shenanigans. This just makes the already difficult act of writing threading code like this even more difficult.

You should not do any of this

Leave it to the pros - find another way to multicore. One easy way? Write a web server, and have it take care of running each request in its own thread. Never communicate with another thread with fields - each field is unique to a connection and is never shared. Instead, inter-webrequest-communications is done through a medium with much simpler to understand sync support. Such a a database, with transactions. Need to tell another thread to show a comment next time it loads? Put the comment in a database, have the other tread read it from there, easy.

Spin loops

This code is disastrous:

while (!status.isDone()) {}

This will make your laptop CPU fan spin up so much your laptop is likely to soon take off for the skies. As it does this, it crowds out all sorts of tasks. You can't busy-wait like this. You need to wait on some lock, use wait(), call lock() on a j.u.c.Lock object - or even Thread.sleep. Ths code will acquire the lock, check the flag, relinquish the lock, and start over, forever, and this means your CPU will soon burn right through the case as your battery drains in 20 minutes.

Your questions

When does thread1 send the interrupt indication for the code in the DownloadFileTask class to validate if (Thread.currentThread().isInterrupted()) return; in the overwritten run(); method?

Never. The only way to interrupt a thread is with t.interrupt(). It's a very complex mechanism that you probably shouldn't use.

If isDone = true, when does while (!status.isDone()){} become false? There are no methods to change the value of isDone.

Immediately. But the keyword is IF isDone = true - and it isn't, because you are either having code that is deadlocked so isDone isn't true, or your spinloop is so busy holding and releasing that lock the other code doesn't get a chance.