Simple test program: Why isn;t Runnable running in the new Thread ?

[Mohammed Azeem]

Hello, I'm applying what I've learned about guarded blocks with a simple app.

PrintWeather object waits until DecideTheWeather, which is running in a new thread, has notified all other threads.
However I can't work out why the second thread is not executing.

Class SharedWeatherData defines :

shared data: static String weather and static boolean hasTheWeatherBeenDecided

Class DecideTheWeather:

decides the weather and updates the weather and boolean flag in SharedWeatherDatashared

Class PrintWeather:

prints the weather if the weather has been decided

Class WeatherProgram contains the main method.

public class SharedWeatherData {
    
    public static String weather;
    public static boolean weatherHasBeenDecided;
  
}

public class WeatherProgram {

    public static void main(String[] args) {
        
        // run the WeatherPrinter object on this (main) thread
        WeatherPrinter weatherPrinter = new WeatherPrinter();
        weatherPrinter.printTheWeather();
        
        WeatherDecider weatherDecider = new WeatherDecider();
        
        // Create a new Runnable to run weatherDecider object in new thread
        Runnable runnable = new Runnable() {
            @Override
            public void run() {
                System.out.println("The runnable is running");
                weatherDecider.decideTheWeather();
                System.out.println("The value of hasTheWeatherBeenDecided is : " + SharedWeatherData.weatherHasBeenDecided);
            }
            
        };
        
        // run weatherDecider on a new thread
        
        Thread thread2 = new Thread(runnable);
        thread2.start();
    }
}

Output:
Inside the printTheWeather() method
Inside the while loop
The value of hasWeatherBeenDecided is : false
The main thread has done simulated work.

public class WeatherPrinter {
    public String weather;
    
    public WeatherPrinter() {
        printTheWeather();
    }
    
    public synchronized void printTheWeather() {
        System.out.println("Inside the printTheWeather() method");
        while (!SharedWeatherData.weatherHasBeenDecided) {
            System.out.println("Inside the while loop");
            System.out.println("The value of hasTheWeatherBeenDecided is : " + SharedWeatherData.weatherHasBeenDecided);
            try {
                // Simulate some work;
                Thread.sleep(3000);
                System.out.println("The main thread has done simulated work");
                wait();
            } catch (InterruptedException ex) {
                Logger.getLogger(WeatherPrinter.class.getName()).log(Level.SEVERE, null, ex);
            }
        }  
        
        System.out.println("The weather is : " + SharedWeatherData.weather);
    }
    
}

public class WeatherDecider {
    
    public String weather;
    
    public synchronized void decideTheWeather() {
        System.out.println("In the method decideTheWeather() ");
        int min = 1;
        int max = 3;
        Random generator = new Random();
        int rndInt = generator.nextInt(max - min) + min;
        System.out.println("The random number was : " + rndInt);
        
        switch(rndInt) {
            case 1:
                weather = "Sunny";
                break;
            case 2:
                weather = "Cloudy";
                break;
            case 3:
                weather = "Raining";
        }
        SharedWeatherData.weather = this.weather;
        SharedWeatherData.weatherHasBeenDecided = true;
        notifyAll();
        
    }
}

[Paul Clapham]

You're asking why line 24 in WeatherProgram does not run, I think. (Let me know if I'm wrong.) Well, it can't run until line 7 completes, and I don't think it ever does.

It's been a very long time since I had to use code which uses threads like this, so I had to look up what the wait() method does.

[Mohammed Azeem]

Good Evening Paul,

I have a horrible sense that I've made a logical mistake in this pattern of inter-thread communication.
To all readers, I will look into it again tomorrow and promise a full answer.

First thoughts on you remarks Paul: the intended flow control was as follows:

The PrintWeather and DecideTheWeather have access to shared data.  PrintWeather must not proceed until the shared data is written to. So, it waits for decideTheWeather to do so.

line 7: main method acquires lock for weatherPrinter by calling weather.printTheWeather() - a synchronized method

line 17: wait() - the main thread voluntarily suspends itself, thereby relinquishing the lock on shared data - the boolean flag weatherHasBeenDecided. It needs to release itself so that the second thread can access the flag

line 24 -25 in WeatherDecider:  the shared data has been set, the second thread notifies all other threads

now, that the main thread is sure that shared data has been set it resumes its run.

.

[Paul Clapham]

Here's what the wait() method does:

The documentation wrote:Causes the current thread to wait until it is awakened, typically by being notified or interrupted.

Since the thread is waiting, it won't be able notify itself that it can continue.

So yeah, a redesign would be a good idea.

[Paul Clapham]

I think part of the trouble is this: You have defined a process which looks like "Do Thing A, then when that's finished Do Thing B". There's really no need to use more than one thread to execute that process. That's not to say you must not use more than one thread, but it does mean that it's going to be hard to twist it into something which does use more than one thread.

[Stephan van Hulst]

I think the program is supposed to demonstrate two threads interacting with each other. The problem is that in this case, one of those threads is initializing the other thread. This is a brittle setup.

You can fix the problem by making the main thread responsible only for setting up and starting the two threads. Importantly, the main thread must NOT call any blocking methods.

There are other issues with your code that are not directly related to your current issue:

Do not instantiate and start threads directly. Instead, use an ExecutorService.

NEVER use static fields to communicate application state between two classes. Static fields must be used for constant values only.

All of your issues can be solved elegantly like this:

public static void main(String... args) {
  var sharedWeatherData = new SharedWeatherData();
  var weatherPrinter    = new WeatherPrinter(sharedWeatherData);
  var weatherDecider    = new WeatherDecider(sharedWeatherData);
  var executor          = Executors.newFixedThreadPool(2);

  try {
    executor.execute(weatherPrinter::printTheWeather);
    executor.execute(weatherDecider::decideTheWeather);
  }

  finally {
    executor.shutdown();
  }
}

In the future, you will be able to do something like this instead:

public static void main(String... args) {
  var sharedWeatherData = new SharedWeatherData();
  var weatherPrinter    = new WeatherPrinter(sharedWeatherData);
  var weatherDecider    = new WeatherDecider(sharedWeatherData);

  try (var scope = new StructuredTaskScope.ShutdownOnFailure()) {
    scope.fork(callable(weatherPrinter::printTheWeather));
    scope.fork(callable(weatherDecider::decideTheWeather));

    scope.join();
  }
}

[Dean Sabado]

Nice program.  How will you tell what the weather is like?  Some other data.  Like a Input/Ouptstream file some where reading the data base.  Would this location or reading work on a program like this.  This is on your home pc or work.  

[Tim Holloway]

Dean Sabado wrote:Nice program.  How will you tell what the weather is like?  Some other data.  Like a Input/Ouptstream file some where reading the data base.  Would this location or reading work on a program like this.  This is on your home pc or work.  

I cannot speak for OP, but I get my weather data from the US National Weather Service API. Equivalents exist for other countries.

A note: making public static variables is not a recommended practice, and doubly-so in a multi-threaded environment. As a general rule, it's best not to be static at all, and if so, to use synchronized accessor methods.

[Mohammed Azeem]

Apologies for the delay in my reply, I code as a hobby in the evenings after my day job.

After much re-study I am now able to identify the faults in the original program and offer a solution that works.

The weather program was a way of modelling inter-thread cooperation:

Two threads sharing the same data (the simplest data structure – a class with field variables and getters/setters)

Establsihing a happens – before relationship between two tasks. One thread’s execution to be contingent upon the second thread completing a required task.
In the weather program, the weather cannot be printed until the decider has decided what it is going to be.

This requires the two threads to share the same monitor (synonymous to ‘lock’).
The lock can be provided by any object, but a convenient object is an instance of Java Object, since it is only its lock we’re interested in.

If like me, you are highly visual, I have attached a sort of flow diagram that shows two threads sharing the same lock in a happens – before relationship.
I explain it as follows:

The first thread acquires the lock and its task (the WeatherPrinter) checks whether a
Boolean flag has been set by the second thread running the WeatherDecider

If not:

It calls wait() allowing the second thread to acquire the monitor’s lock.

The second thread’s task (WeatherDecider) can then decide the weather and set the flag to show it has been decided.
The task then issues a notify() on the monitor and releases the lock.

The first thread re-aquires the lock (remember it’s the same lock)

It sees that the weather flag is set and proceeds to print the weather.

If yes:

It reads the weather string and prints it.

Is the data in WeatherData, guarded from being changed by objects other than WeatherData?  I believer so.
The object reference weatherData is declared final, meaning that it will always refer to a single unique WeatherData object.
Therefore, passing the weatherData reference always passes the same object.

Finally, thank you for taking an interest in the post. Maybe I should payback by responding to other people's posts. But I implore you gently - please make your code readable, it's half the battle !

Class WeatherData

public class WeatherData {
    private String weather;
    private boolean hasWeatherBeenDecided;
    
    
    public String getWeather() {
        return this.weather;
    }
    
    public boolean getHasWeatherBeenDecided() {
        return this.hasWeatherBeenDecided;
    }
    
    public void setWeather(String weather) {
        this.weather = weather;
    }
    
    public void setHasWeatherBeenDecided(boolean hasWeatherBeenDecided) {
        this.hasWeatherBeenDecided = hasWeatherBeenDecided;
    }
}

Class WeatherPrinter

public class WeatherPrinter implements Runnable {
    
   
    final WeatherData weatherData;
    final Object monitor;
    
    public WeatherPrinter(WeatherData weatherData, Object monitor) {
        this.weatherData = weatherData;
        this.monitor = monitor;
    }
    
    @Override
    public void run() {
        printWeather();     
    }
    
    
    public void printWeather() {
        synchronized(monitor) {
            while (weatherData.getHasWeatherBeenDecided() == false) {
                try {
                    monitor.wait();
                } catch (InterruptedException ex) {
                     System.out.println("An InterruptedException occurred");
                }
            }
            
            System.out.println("The printed weather is : " + weatherData.getWeather());
            weatherData.setHasWeatherBeenDecided(true);
        }
    }
    
}

Class WeatherDecider

public class WeatherDecider implements Runnable {
    
    final WeatherData weatherData;
    final Object monitor;
    
    public WeatherDecider (WeatherData weatherData, Object monitor) {
        this.weatherData = weatherData;
        this.monitor = monitor;
    }
    
    @Override
    public void run() {
        decideWeather();
    }
    
    public void decideWeather() {
        synchronized(monitor) {
            if (weatherData.getHasWeatherBeenDecided() == false) {
                int min = 1;
                int max = 3;
                Random generator = new Random();
                int rndInt = generator.nextInt(max) + min;
                
                switch (rndInt) {
                case 1:
                    weatherData.setWeather("Sunny");
                    break;
                case 2:
                    weatherData.setWeather("Cloudy");
                    break;
                case 3:
                    weatherData.setWeather("Raining");
                }
            }
            
            weatherData.setHasWeatherBeenDecided(true);
            System.out.println("Today's weather has been decided as : " + weatherData.getWeather());
            monitor.notify();
        }
    }
}

Main method class

public class WeatherProgram_3 {

    public static void main(String[] args) {
        
        // object reference weatherData cannot be assigned to any other object.
        // all the weatherData references passed into various methods will point to the one same object
        final WeatherData weatherData = new WeatherData();
        
        final Object monitor = new Object();
        
        Thread task1 = new Thread(new WeatherPrinter(weatherData, monitor));
        task1.start();
        
        Thread task2 = new Thread(new WeatherDecider(weatherData, monitor));
        task2.start();
    }
}

Typical output:

Today's weather has been decided as : Sunny
The printed weather is : Sunny

[Stephan van Hulst]

Yes, your code is correct.

I have a few comments to help you improve your code though:

Make your classes final unless you have a good reason not to.

Don't make your classes and methods public unless you have a good reason. Making them public by default is a bad habit.

Make your fields private.

You can remove the hasWeatherBeenDecided field by relying on a null value for weather instead.

Validate your constructor and method parameters.

Prefer enum constants over string constants.

Don't catch exceptions unless you have a good way of handling them.

You can use Callable instead of Runnable to throw exceptions from a task.

Instead of creating services like Random on the fly, inject them into your constructors.

Don't print to System.out unless it's part of your business logic. Use loggers instead.

You can simplify your application a little bit by moving your synchronization from your runnables to the WeatherData class.

Using monitors and wait() and notify() is a bit old-fashioned. These days we prefer locks and conditions.

Use an executor service instead of creating your threads manually.

package com.example.weather;

import java.util.*;

/**
 * Constants describing different kinds of weather.
 */
enum Weather {

  SUNNY   ("Sunny"),
  CLOUDY  ("Cloudy"),
  RAINING ("Raining");

  private final String description;

  private Weather(String description) {
    this.description = description;
  }

  @Override
  public String toString() {
    return description;
  }

  /**
   * Returns a random weather constant.
   *
   * @param generator the random number generator used to select a random weather constant.
   *
   * @return Any of the weather constants of this class.
   *
   * @throws IllegalArgumentException if {@code generator} is {@code null}.
   */
  static Weather random(Random generator) {
    if (generator == null) {
      throw new IllegalArgumentException("The generator may not be null.");
    }

    var values = values();

    var index = generator.nextInt(values.length);

    return values[index];
  }
}

package com.example.weather;

import java.lang.System.Logger;
import java.lang.System.Logger.Level;
import java.util.*;
import java.util.concurrent.locks.*;

/**
 * Thread-safe container for weather data.
 */
final class WeatherData {

  private static final Logger LOGGER = System.getLogger(WeatherData.class.getName());

  private final Lock lock = new ReentrantLock();

  private final Condition weatherSetCondition = lock.newCondition();

  private final Condition weatherResetCondition = lock.newCondition();

  private volatile Weather weather;

  /**
   * Gets the current weather.
   *
   * @return An {@code Optional} of the current weather if it has been
   *         {@linkplain #setWeather(com.example.weather.Weather) set}, or
   *         {@link Optional#empty()} if no weather has been set or if it has been
   *         {@linkplain #getAndResetWeather() reset}.
   */
  Optional<Weather> weather() {
    return Optional.ofNullable(weather);
  }

  /**
   * Gets whether the current weather has been set.
   *
   * @return {@code true} if the current weather has been {@linkplain #setWeather(com.example.weather.Weather) set};
   *          {@code false} if it has not been set or if it has been {@linkplain #getAndResetWeather() reset}.
   */
  boolean isWeatherSet() {
    return weather != null;
  }

  /**
   * Sets the current weather.
   *
   * If the weather {@linkplain #isWeatherSet() is already set},
   * this method blocks until another thread {@linkplain #getAndResetWeather() resets the weather}.
   *
   * @param weather the weather to set.
   *
   * @throws IllegalArgumentException if {@code weather} is {@code null}.
   * @throws InterruptedException     if the current thread is interrupted while waiting for the weather to reset.
   */
  void setWeather(Weather weather) throws InterruptedException {
    if (weather == null) {
      throw new IllegalArgumentException("The weather may not be null.");
    }

    lock.lockInterruptibly();

    try {
      while (isWeatherSet()) {
        LOGGER.log(Level.INFO, "Waiting for weather to be reset.");
        weatherResetCondition.await();
      }

      LOGGER.log(Level.INFO, "Setting weather.");

      this.weather = weather;
      
      weatherSetCondition.signalAll();
    }

    finally {
      lock.unlock();
    }
  }

  /**
   * Gets and resets the current weather.
   *
   * If the weather {@linkplain #isWeatherSet() hasn't been set},
   * this method blocks until another thread {@linkplain #setWeather(com.example.weather.Weather) sets the weather}.
   *
   * @return The weather that has previously been set.
   *
   * @throws InterruptedException if the current thread is interrupted while waiting for the weather to be set.
   */
  Weather getAndResetWeather() throws InterruptedException {
    lock.lockInterruptibly();

    try {
      while (!isWeatherSet()) {
        LOGGER.log(Level.INFO, "Waiting for weather to be set.");
        weatherSetCondition.await();
      }

      LOGGER.log(Level.INFO, "Getting and resetting weather.");

      var result = weather;
      weather = null;

      weatherResetCondition.signalAll();

      return result;
    }

    finally {
      lock.unlock();
    }
  }
}

package com.example.weather;

import java.util.*;
import java.util.concurrent.*;

/**
 * A callable task that decides the current weather.
 */
final class WeatherDecider implements Callable<Void> {

  private final WeatherData weatherData;

  private final Random generator;

  /**
   * Initializes this weather decider.
   *
   * @param weatherData the container in which to set the weather.
   * @param generator   the random number generator used to decide the weather at random.
   *
   * @throws IllegalArgumentException if {@code weatherData} is {@code null}.
   * @throws IllegalArgumentException if {@code generator} is {@code null}.
   */
  WeatherDecider(WeatherData weatherData, Random generator) {
    if (weatherData == null) {
      throw new IllegalArgumentException("The weather data may not be null.");
    }

    if (generator == null) {
      throw new IllegalArgumentException("The generator may not be null.");
    }

    this.weatherData = weatherData;
    this.generator   = generator;
  }

  /**
   * Decides the current weather.
   *
   * This call blocks as long as the weather is already decided.
   *
   * @return {@code null}.
   *
   * @throws InterruptedException if the current thread is interrupted while waiting for the weather to reset.
   */
  @Override
  public Void call() throws InterruptedException {
    decideWeather();
    return null;
  }

  private void decideWeather() throws InterruptedException {
    var weather = Weather.random(generator);
    weatherData.setWeather(weather);
  }
}

package com.example.weather;

import java.io.*;
import java.util.concurrent.*;

/**
 * A callable task that prints the current weather.
 */
final class WeatherPrinter implements Callable<Void> {

  private final WeatherData weatherData;

  private final PrintWriter writer;

  /**
   * Initializes this weather printer.
   *
   * @param weatherData the container from which to retrieve the current weather.
   * @param writer      the writer used to print the current weather.
   *
   * @throws IllegalArgumentException if {@code weatherData} is {@code null}.
   * @throws IllegalArgumentException if {@code writer} is {@code null}.
   */
  WeatherPrinter(WeatherData weatherData, PrintWriter writer) {
    if (weatherData == null) {
      throw new IllegalArgumentException("The weather data may not be null.");
    }

    if (writer == null) {
      throw new IllegalArgumentException("The writer may not be null.");
    }

    this.weatherData = weatherData;
    this.writer      = writer;
  }

  /**
   * Prints the current weather.
   *
   * This call blocks until the weather is decided.
   *
   * @return {@code null}.
   *
   * @throws InterruptedException if the current thread is interrupted while waiting for the weather to be decided.
   */
  @Override
  public Void call() throws InterruptedException {
    printWeather();
    return null;
  }

  private void printWeather() throws InterruptedException {
    var weather = weatherData.getAndResetWeather();
    writer.println("The printed weather is: " + weather);
  }
}

package com.example.weather;

import java.io.*;
import java.util.*;
import java.util.concurrent.*;

final class WeatherApplication {

  public static void main(String... args) {
    var generator      = new Random();
    var writer         = new PrintWriter(System.out, true);
    var weatherData    = new WeatherData();
    var weatherDecider = new WeatherDecider(weatherData, generator);
    var weatherPrinter = new WeatherPrinter(weatherData, writer);
    var executor       = Executors.newFixedThreadPool(2);

    try {
      executor.submit(weatherPrinter);
      executor.submit(weatherDecider);
    }

    finally {
      executor.shutdown();
    }
  }
}

[Mohammed Azeem]

Thank you Stephan,

I'll read up on  ENUMS - I'm aware of them but have neglected them.
Yes, I'm in the bad habit of declaring fields public - inexplicable really.
I'll study your use of the Executor service .