Thread extending, access method vs filed value

Question

So I am trying to write a simple class that is extending Thread class. While writing it I found a funny and confusing casualty and I would be very grateful if someone explain me

why this code does work:

@Override
public void run(){
    startTime = System.currentTimeMillis();
    running = true;
    while (isRunning()) {}
}

but this code doesn't:

@Override
public void run(){
    startTime = System.currentTimeMillis();
    running = true;
    while (running) {}
}

with overridden interrupt method:

@Override
public void interrupt(){
    running = false;
    Long endTime = System.currentTimeMillis();
    int time = Math.round(endTime - startTime);
    out.println(time);
}

BTW isRunning() is just a simple getter witch returns boolean value of running.

Please add a [mcve] people answering shouldn't have to guess what you're doing. — Nathan Hughes, Jun 22 '22 at 13:41
What do you mean by doesn't work? That you cannot stop the running thread? If running is not declared volatile then the value is probably getting cached. If you declare it volatile it should work better. — matt, Jun 22 '22 at 13:44
@matt, maybe not "cached." The compiler is allowed to transform `running=true; while(running){...}` into `while(true){...}` if it can prove that the `...` won't change the value of `running`. In this case, the proof is trivially easy because the `...` does nothing at all. — Solomon Slow, Jun 22 '22 at 14:18

score 0 · Accepted Answer · answered Jun 22 '22 at 13:42

What you have here is a race condition. In the particular case of Java, we can read a direct definition from the JLS

Two accesses to (reads of or writes to) the same variable are said to be conflicting if at least one of the accesses is a write.

You have a single variable and you have two threads accessing it at the same time, where one of those threads is writing to it. To protect this data, you need to synchronize both accesses. Generally, it's advised to synchronize on a private instance variable of type Object, so that no one else has a reference to that variable. (The default behavior of synchronized methods is to synchronize on this, which can cause problems if someone else who has a reference to this synchronizes on it as well)

private boolean running;
private Object lock;

public MyClass() {
  lock = new Object();
}

...

@Override
public void run() {
  startTime = System.currentTimeMillis();
  synchronized (lock) {
    running = true;
  }
  while (isRunning()) {}
}

public boolean isRunning() {
  synchronized (lock) {
    return running;
  }
}

@Override
public void interrupt() {
  synchronized (lock) {
    running = false;
  }
}

And, as has already been pointed out in the comments, it is generally frowned upon to subclass Thread when you're doing things like this. Consider making your run() in a Runnable subclass and simply use the Thread constructor.

I would not call this a race condition. Although synchronized would probably fix the issue. They have a tight loop and a variable that is getting read, if that variable isn't volatile or in a synchronized block the JVM can used a cached value. — matt, Jun 22 '22 at 13:47

Thread extending, access method vs filed value

1 Answers1