Does this code for deterministic execution of java threads look correct from a concurrency point of view?

Question

I wanted a way to have multiple threads, but to have only one thread running at a time, controlling which thread ran when and how much code each thread would run at a time.

The solution I've arrived at is to have a controller thread and worker threads. The controller thread cedes control in round robin fashion to each worker thread in turn. The worker threads contain 'cede points' at which they return control back to the controller.

The code uses 'wait' and 'notify', which I know is something of a rat's nest. I think I've done a reasonable job of handling concurrency problems, and the code seems to work, but I'm wondering if there are any hidden problems waiting to bite me. I also thought I'd share this code because I've seen other questions suggesting that deterministic thread control is hard/impossible to do, but this solution looks like it could be useful for certain use cases.

Thanks.

EDIT3: Amended, working code at the end of this post

import java.util.Vector;

public class Controller extends Thread{

    private Vector<Thread> threads = new Vector<>();
    
    public void addThread(Thread thread){
        System.out.println("Controller adding thread of "+thread);
        threads.add(thread);
    }
    
    @Override
    public void run(){
        // Loop over the threads I manage, running each one in turn until 
        // it hands back control to me
        try {
            while(true){
                for (int i=0; i<threads.size(); i++){
                    synchronized(threads.get(i)){
                        threads.get(i).notify(); // Wake up the worker thread
                        threads.get(i).wait(); // And request to be woken by the worker thread when it's done
                    }                   
                    Thread.sleep(1000);
                }
            }
        }
        catch (Exception e){
            e.printStackTrace();
        }
    }
}

public class WorkerThread extends Thread{

    private Controller controller;
    private boolean registered = false;
    
    public WorkerThread(Controller controller){
        this.controller = controller;
    }
    
    protected void cedeControl() throws InterruptedException{
        synchronized(this){
            if (!registered){
                controller.addThread(this);
                registered = true;
            }
            else {
                this.notify(); // Give control back to the controller
            }
            this.wait(); // And request to be awoken again when the controller wants
        }
    }
}

public class ExampleThread extends WorkerThread{

    public ExampleThread(Controller controller){
        super(controller);
    }
    
    @Override
    public void run(){              
        try{
            while (true){
                cedeControl();
                System.out.println("Thread "+this+" running part 1");
                cedeControl();
                System.out.println("Thread "+this+" running part 2");
                cedeControl();
                System.out.println("Thread "+this+" running part 3");
            }
        }
        catch (Exception e){
            e.printStackTrace();
        }
    }   
}

public class Test{

    public static void main(String[] args){
        Test t = new Test();
        t.run();
    }
    
    public void run(){
        Controller controller = new Controller();
        controller.start();
        
        for (int i=0; i<3; i++){
            Thread exampleThread = new ExampleThread(controller);
            exampleThread.start();
        }
    }
}

Output:

Controller adding thread of Thread[Thread-1,5,main]
Controller adding thread of Thread[Thread-2,5,main]
Controller adding thread of Thread[Thread-3,5,main]
Thread Thread[Thread-1,5,main] running part 1
Thread Thread[Thread-2,5,main] running part 1
Thread Thread[Thread-3,5,main] running part 1
Thread Thread[Thread-1,5,main] running part 2
Thread Thread[Thread-2,5,main] running part 2
Thread Thread[Thread-3,5,main] running part 2
Thread Thread[Thread-1,5,main] running part 3
Thread Thread[Thread-2,5,main] running part 3
Thread Thread[Thread-3,5,main] running part 3
Thread Thread[Thread-1,5,main] running part 1
Thread Thread[Thread-2,5,main] running part 1
Thread Thread[Thread-3,5,main] running part 1
etc

EDIT: I've rewritten it to use a semaphore. Thanks to @GPI for the suggestion. It looks to work fine. I'm slightly concerned that the same thread could reacquire the semaphore if the other threads haven't yet tried to acquire it for some reason, but it's certainly a lot simpler than my wait/notify solution.

EDIT2: Hmm, as I feared the semaphore solution isn't robust. Looking through its output I found this:

Thread Thread[Thread-0,5,main] running part 3
Thread Thread[Thread-2,5,main] running part 2
Thread Thread[Thread-1,5,main] running part 3
Thread Thread[Thread-0,5,main] running part 1
Thread Thread[Thread-2,5,main] running part 3
Thread Thread[Thread-1,5,main] running part 1
Thread Thread[Thread-0,5,main] running part 2
Thread Thread[Thread-2,5,main] running part 1
Thread Thread[Thread-1,5,main] running part 2

For reference, this is the semaphore code:

import java.util.concurrent.Semaphore;

public class ExampleThread extends Thread{

    private Semaphore semaphore;
    
    public ExampleThread(Semaphore semaphore){
        this.semaphore = semaphore;
    }
    @Override
    public void run(){              
        try{
            while (true){
                semaphore.acquire();
                System.out.println("Thread "+this+" running part 1");
                semaphore.release();
                semaphore.acquire();
                System.out.println("Thread "+this+" running part 2");
                semaphore.release();
                semaphore.acquire();
                System.out.println("Thread "+this+" running part 3");
                semaphore.release();
            }
        }
        catch (Exception e){
            e.printStackTrace();
        }
    }   
}

public class Test{

    public static void main(String[] args){
        Test t = new Test();
        t.run();
    }
    
    public void run(){
        Semaphore semaphore = new Semaphore(1, true); // The true makes it fair
        for (int i=0; i<3; i++){
            Thread exampleThread = new ExampleThread(semaphore);
            exampleThread.start();
        }
    }
}

Output:
Thread Thread[Thread-0,5,main] running part 1
Thread Thread[Thread-1,5,main] running part 1
Thread Thread[Thread-2,5,main] running part 1
Thread Thread[Thread-0,5,main] running part 2
Thread Thread[Thread-1,5,main] running part 2
Thread Thread[Thread-2,5,main] running part 2
Thread Thread[Thread-0,5,main] running part 3
Thread Thread[Thread-1,5,main] running part 3
Thread Thread[Thread-2,5,main] running part 3
Thread Thread[Thread-0,5,main] running part 1
Thread Thread[Thread-1,5,main] running part 1
Thread Thread[Thread-2,5,main] running part 1
Thread Thread[Thread-0,5,main] running part 2
Thread Thread[Thread-1,5,main] running part 2
Thread Thread[Thread-2,5,main] running part 2
Thread Thread[Thread-0,5,main] running part 3
Thread Thread[Thread-1,5,main] running part 3
Thread Thread[Thread-2,5,main] running part 3
etc

EDIT3: I have what I consider to be a working version using wait and notify, with a registration method and guarding against spurious wakeups. Code is:

import java.util.Vector;

public class Controller extends Thread{

    private Vector<Thread> threads = new Vector<>();
    private volatile Thread currentThread; // Needed to cope with spurious wakeups 
                                // see https://docs.oracle.com/javase/8/docs/api/java/lang/Object.html#wait-long-
    
    public void addThread(Thread thread){
        System.out.println("Controller adding thread of "+thread);
        threads.add(thread);
    }
    
    public Thread getCurrentThread(){
        return currentThread;
    }

    public void setCurrentThread(Thread currentThread){
        this.currentThread=currentThread;
    }


    @Override
    public void run(){
        // Loop over the threads I manage, running each one in turn until 
        // it hands back control to me
        try {
            while(true){
                for (int i=0; i<threads.size(); i++){
                    synchronized(threads.get(i)){
                        setCurrentThread(threads.get(i));
                        threads.get(i).notify(); // Wake up the worker thread
                        do {
                            threads.get(i).wait(); // And pause myself
                        } while (getCurrentThread() != this); // Spurious wakeup guard
                        // see https://docs.oracle.com/javase/8/docs/api/java/lang/Object.html#wait-long-
                    }                   
                    Thread.sleep(1000);
                }
            }
        }
        catch (Exception e){
            e.printStackTrace();
        }
    }
}

public class WorkerThread extends Thread{

    private Controller controller;
    private boolean registered = false;
    
    public WorkerThread(Controller controller){
        this.controller = controller;
    }
    
    protected void register() throws InterruptedException{
        synchronized(this){
            if (!registered){
                controller.addThread(this);
                registered = true;
                do {
                    this.wait(); // Request to be woken when the controller calls on me
                } while (controller.getCurrentThread() != this); // Spurious wakeup guard
                // see https://docs.oracle.com/javase/8/docs/api/java/lang/Object.html#wait-long-                   
            }
            else {
                throw new RuntimeException("Thread has already been registered!");
            }
        }
    }
    
    protected void cedeControl() throws InterruptedException{
        synchronized(this){
            if (!registered){
                throw new RuntimeException("Thread has not been registered!");
            }
            else {
                controller.setCurrentThread(controller);
                this.notify(); // Give control back to the controller
                do {
                    this.wait(); // And request to be awoken again when the controller wants
                } while (controller.getCurrentThread() != this); // Spurious wakeup guard
                // see https://docs.oracle.com/javase/8/docs/api/java/lang/Object.html#wait-long-                   
            }                       
        }
    }   
}

public class ExampleThread extends WorkerThread{

    public ExampleThread(Controller controller){
        super(controller);
    }
    
    @Override
    public void run(){              
        try{
            register(); // Put thread under the control of the controller
            while (true){
                System.out.println("Thread "+this+" running part 1");
                cedeControl();
                System.out.println("Thread "+this+" running part 2");
                cedeControl();
                System.out.println("Thread "+this+" running part 3");
                cedeControl();
            }
        }
        catch (Exception e){
            e.printStackTrace();
        }
    }   
}

public class Test{

    public static void main(String[] args){
        Test t = new Test();
        t.run();
    }
    
    public void run(){
        Controller controller = new Controller();
        controller.start();
        
        for (int i=0; i<3; i++){
            Thread exampleThread = new ExampleThread(controller);
            exampleThread.start();
        }
    }
}

Output:
Controller adding thread of Thread[Thread-1,5,main]
Controller adding thread of Thread[Thread-2,5,main]
Controller adding thread of Thread[Thread-3,5,main]
Thread Thread[Thread-1,5,main] running part 1
Thread Thread[Thread-2,5,main] running part 1
Thread Thread[Thread-3,5,main] running part 1
Thread Thread[Thread-1,5,main] running part 2
Thread Thread[Thread-2,5,main] running part 2
Thread Thread[Thread-3,5,main] running part 2
Thread Thread[Thread-1,5,main] running part 3
Thread Thread[Thread-2,5,main] running part 3
Thread Thread[Thread-3,5,main] running part 3
Thread Thread[Thread-1,5,main] running part 1
Thread Thread[Thread-2,5,main] running part 1
Thread Thread[Thread-3,5,main] running part 1
etc

The registration pattern is off. If a worker thread starts, and does not call `cedeControl()` as its first action, then it can work outside of the controler's sight for as long as it wishes. I feel you should, for a start, make registration the first step of any worker's run. — GPI, Jul 29 '20 at 14:03
I realize that I've forgotten to cope with spurious wakeups, where threads can spontaneously wake up without having been notified. Would be simple to add in a volatile variable that holds which thread is supposed to be running so that waking threads can check if it's really their turn. — Bruce, Jul 29 '20 at 14:10
A second flaw : what happens when a thread is done ? It should be unregistered, I guess, because it will never notify back. — GPI, Jul 29 '20 at 14:19
Not really an answer, but have you actually achieved anything non-trivial ? In your design the controler does not have a say in how long workers run, it's the workers that decide when to release control. The controler merely handle a permit to work on a sequential basis. Therefore you could remove each and every thread and lock in your design, and only have a worker with 2 methods : "performNextPieceOfWork" and "isDone". The controler would be a for loop over all workers, calling the first and checking the second after (to derigster). A.K.A. "One thread is no thread." — GPI, Jul 29 '20 at 14:20
@GPI Thanks for point about unregistration - yep, that would be useful. — Bruce, Jul 29 '20 at 14:22
@GPI As it stands, good point, yes it is trivial, but my intention is to have code that can be run in both deterministic and non-deterministic mode. I want to simulate independent modules of the brain, and show what happens when they run truly independently, but I also want to be able to optionally run the simulation in a slowed down, deterministic step-by-step fashion, with the various modules each being advanced step by step in round robin fashion. For non-deterministic mode I think as simple as making cedeControl() do nothing. — Bruce, Jul 29 '20 at 14:25
I would argue a fair Semaphore would do the same. Sequential mode : 1 permit. Parallel mode : N permits. Fairness would ensure the sequential and in order of declaration nature of work. — GPI, Jul 29 '20 at 14:32
@GPI Ah thanks! Was not familiar with Semaphores. That looks very promising and much simpler! Will rework. — Bruce, Jul 29 '20 at 14:36

score 0 · Answer 1 · answered Jul 29 '20 at 21:15

0

You use notify when you change shared state and need to let other threads wake up to notice that change. You use wait when shared state isn't correct and you need another thread to change that shared state before you can make forward progress. You don't have any shared state, so your use of notify and wait in incorrect.

synchronized(threads.get(i)){
  threads.get(i).notify(); // Wake up the worker thread
  threads.get(i).wait(); // And request to be woken by the worker thread when it's done
}

You synchronize on threads.get(i). But no shared state is accessed or modified in this block. So what does the synchronization protect? You call notify, but you haven't modified any shared state, so there is nothing to notify about. You call wait, but then after wait returns, you don't even check if the thing you were waiting for happened.

You need to create some shared state. At minimum, a boolean for each thread that indicates whether that thread can run or not. The controller can set it to true and notify the thread that the shared state changed. The thread can set it to false and then wait for it to become true again. Then, your synchronized blocks have some shared state to protect and so it makes sense.

answered Jul 29 '20 at 21:15

David Schwartz

179,497
17
214
278

Thank you for this answer. I'm not clear why I need shared state. I'm using synchronized to obtain the monitor, then I'm using wait and notify purely for their side effects of causing threads to wait or run. I want thread A to wait while thread B runs, then thread B to wait while thread A runs. For each thread to be able to get the monitor I need to write the switchover point in a synchronised block, I think? There's more explanations of why I would want to do this in the comments. The semaphore solution will probably be ok, but I think my solution guarantees round-robinning. – Bruce Jul 29 '20 at 21:26
You can't use `notify` and `wait` purely for their side effects. It's quite complicated to explain why, but you absolutely cannot do that. The `synchronized` block *must* protect some shared state that you are waiting for, otherwise the `wait` function has no way to know when to stop waiting. The `notify` function *doesn't* change any state to a "stop waiting" state. – David Schwartz Jul 29 '20 at 21:45
Thanks. But if what you say is correct, why does my code work? My understanding is that the notify function absolutely changes the state of another thread. From the javadoc of notify: "Wakes up a single thread that is waiting on this object's monitor." – Bruce Jul 29 '20 at 21:52
@Bruce It works by luck. The issue isn't with `notify`, it's with `wait`. The `wait` function has no idea what it's waiting for and thus no idea when to keep waiting. See [here](https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/Condition.html) . Read the first paragraph under "Implementation Considerations". But the real issue is subtly different. Spurious wakeups are just the easy way to show that the code is broken without getting into the really ugly details. – David Schwartz Jul 29 '20 at 22:04
Thanks. Looks like there's more stuff I need to dig into. I'm going to be putting in a fix for spurious wakeups which will involve a shared variable anyway so that the thread knows if it's really supposed to run, so I guess that will fix the shared state issue you raise. However, I would of course ideally like to understand the underlying issues. Thanks for your responses. – Bruce Jul 29 '20 at 22:14
@Bruce The underlying issue is that the `synchronized` block has to protect the shared state that is being `wait`ed for *because* the purpose of `wait` is to provide an atomic "unlock and wait" operation. That makes no sense unless the "unlock" releases the lock that protects the shared state it is waiting for. Again, how are you expecting `wait` to know when to stop waiting? Say the implementation needs the thread to do something and has to interrupt the wait for the thread to do it. How should the thread know whether to continue waiting afterwards? These are low-level, fast sync primitives. – David Schwartz Jul 29 '20 at 22:30
Apologies if I'm being obtuse, but the javadoc on the Object class for wait() and notify() makes no mention of your shared state requirement. 'Wait' simply puts a thread to sleep pending its wakeup by a number of means, one of which is a call to 'notify' by another thread. The design purpose of wait and notify may be to synchronize state but that doesn't mean that you have to use them that way, any more than you have to use an off road vehicle exclusively off road because that's what it was designed for. I want two threads with no shared state to run alternately, never together. – Bruce Jul 30 '20 at 06:01

Does this code for deterministic execution of java threads look correct from a concurrency point of view?

1 Answers1