Why is my boolean not being changed?

Question

So I'm trying to create a client/server program. I want to know when my client disconnects of his own accord, so I've setup a heartbeat system. Every 6 seconds my client sends a ping to my server, if the client doesn't send a ping for a total of 30 seconds the client is considered disconnected and removed from the current connections list (for which I plan to implement a GUI). Or at least, that's the plan.

ConnectionManager.java

public class ConnectionManager implements Runnable{

static Socket connection;

private ArrayList<Thread> allConnections;
private ArrayList<Connection> allConnectionList;
private ServerSocket server;
private int id = 0;

public ConnectionManager() {
    allConnections = new ArrayList<Thread>();
    allConnectionList = new ArrayList<Connection>();
}


@Override
public void run() {
    try {
        server = new ServerSocket(5555);
        System.out.println("Server is running!");
        while(true) {
            connection = server.accept();
            Connection a = new Connection(connection, id);
            Runnable runnable = a;
            allConnectionList.add(a);
            allConnections.add(new Thread(runnable));
            allConnections.get(allConnections.size() - 1).start();
            id++;
        }
    } catch (IOException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }
}

public void removeConnection(int id) {
    allConnections.remove(id);
    allConnectionList.remove(id);
}

Connection.java

public class Connection implements Runnable {

private Socket a;
public boolean amIActive;
private int id;

public Connection(Socket a, int id) {
    amIActive = true;
    this.a = a;
    this.id = id;
}

public void onConnect() {
    try {
        String TimeStamp = new java.util.Date().toString();
        String formattedAddress = a.getInetAddress().toString().replace("/", "");
        System.out.println("Received connection from: " + formattedAddress + " at " + TimeStamp);
        Runnable runnable = new ConnectionListener(this);
        Thread connectionThread = new Thread(runnable);
        connectionThread.start();
        String returnCode = "Server repsonded to " + a.getInetAddress().toString().replace("/", "") + " at "+ TimeStamp + (char) 13;
        BufferedOutputStream os = new BufferedOutputStream(a.getOutputStream());
        OutputStreamWriter osw = new OutputStreamWriter(os, "US-ASCII");
        osw.write(returnCode);
        osw.flush();

    } catch (IOException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }
}

@Override
public void run() {
    onConnect();
    System.out.println("We got this far!");
    while(amIActive) {
        whileTrue();
    }
    System.out.println("This code never gets run because we get stuck in the while loop above");
    Main.b.removeConnection(id);
    System.out.println("Connection was closed from " + a.getInetAddress());
}

public void setOffline(boolean state) {
    this.amIActive = state;
}

public void whileTrue() {
}

public Socket getSocket() {
    return a;
}

ConnectionListener.java

public class ConnectionListener implements Runnable{

public Connection myConnection;
public boolean receivedHeartbeat;
public int missedHeartbeats = 0;

public ConnectionListener(Connection a) {
    this.myConnection = a;
}


@Override
public void run() {

    Runnable runnable = new Heartbeat(this);
    Thread thread = new Thread(runnable);
    thread.start();

    while(myConnection.amIActive) {
        try {
            BufferedInputStream is;
            is = new BufferedInputStream(myConnection.getSocket().getInputStream());
            InputStreamReader isr = new InputStreamReader(is);
            StringBuffer process = new StringBuffer();
            int character;

            while((character = isr.read()) != 13) { //GETTING STUCK HERE BECAUSE STUPID.
                if(character == -1) {
                    myConnection.setOffline(true);
                } else { 
                    process.append((char)character);
                }
            }
            handleInput(process);
        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}

public void handleInput(StringBuffer process) {
    String messageSent = process.toString();
    if(messageSent.equals("Ping!")) {
        receivedHeartbeat = true;
    }
}

Heartbeat.java

public class Heartbeat implements Runnable{

private ConnectionListener b;

public Heartbeat(ConnectionListener a) {
    b = a;
}

@Override
public void run() {
    while(true) {
        try {
            Thread.sleep(1000);
            if(b.missedHeartbeats > 5) {
                b.myConnection.amIActive = false;
                System.out.println("Setting amIActiveToFalse!");
            }
            if(b.receivedHeartbeat) {
                b.receivedHeartbeat = false;
            } else {
                b.missedHeartbeats++;
            }
        } catch (InterruptedException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
    }

}

My console is spammed with System.out.println("Setting amIActiveToFalse!"); from Heartbeat.java. But the while loop in Connection.java keeps running. I believe this might be something to do with my threading, but I can't figure it out.

Answer 1

There are a couple issues I saw while debugging the code you posted, but I was able to successfully get the heartbeat functionality working.

1. In the Connection Listener class I don't think the if statement with .equals("Ping!") will match, because of the newline character at the end of each line.
2. In the Connection Listener class I would probably put the socket's Input Stream at the top of the loop not inside the loop. (I don't think this will break it but it's probably nicer this way)

ConnectionListener Updates:

public void run() {

  Runnable runnable = new Heartbeat(this);
  Thread thread = new Thread(runnable);
  thread.start();

  BufferedReader br = null;
  try {
  //is = new BufferedInputStream(myConnection.getSocket().getInputStream());
    br = new BufferedReader(new InputStreamReader(myConnection.getSocket().getInputStream()));
    } catch (IOException e1) {
      // TODO Auto-generated catch block
      e1.printStackTrace();
    }

  while(myConnection.amIActive) {
    try {
      String processLine = br.readLine();
        System.out.println("handleInput:" + processLine);
        handleInput(processLine);
    } catch (Exception e) {
      System.out.println("Exception!");
        e.printStackTrace();
    }
  }
}

public void handleInput(String messageSent) {
  if(messageSent.startsWith("Ping!")) { //Need to use startsWith, or add newline character
    receivedHeartbeat = true;
    System.out.println("receivedHeartbeat!");
  }
}

3. Also, in your Heartbeat class make sure you reset the missedHeartbeats counter to 0 on true:

if(b.receivedHeartbeat) {
  b.receivedHeartbeat = false;
  b.missedHeartbeats = 0;
} else {
  b.missedHeartbeats++;
}

Answer 2

The trivial answer to this is: make the variable volatile.

Without this, it is allowed for the thread changing the value to basically keep its updates in cache, committing them to main memory some time later.

This allows threaded code to run much faster, since it can keep its variables in cache rather than having to fetch from main memory. However, the consequence of this is that other threads don't see the update.

Making the variable volatile prevents this from happening: a thread always reads the value from main memory, and writes are immediately committed.

I say that this is the trivial answer because it doesn't necessarily fix all of your problems. There may also be an atomicity issue: in between one thread reading the variable and writing it again, another thread might sneak in and change its value, which may or may not put the first thread into an undefined state from the perspective of its invariants.

Specifically:

if(b.receivedHeartbeat) { b.receivedHeartbeat = false;

It is possible that some other thread can change b.receivedHeartbeat to false after this thread evaluates it to true, so this iteration is erroneously counted as a "non-missed" heartbeat.

This can be fixed by making the variable a (non-volatile) AtomicBoolean, on which there is an atomic compare-and-set method, which avoids such race conditions.

Java Concurrency In Practice is a great reference on these issues, I wholeheartedly recommend it. Look for the topics "visibility" and "atomicity".

Also read the advanced chapter on the Java Memory Model. That made me doubt myself at first, but made me a much stronger programmer after I digested it.

Answer 3

When you have a non-volatile variable, there is no guarentee of visability of a change in one thread to another. In particular, if the JVM detects that a thread doesn't alter a boolean it can inline it, meaning you will never see the value change.

The simple solution is to make the boolean volatile and it will not be inlined and one thread will see when another changes it.

For more details http://vanillajava.blogspot.com/2012/01/demonstrating-when-volatile-is-required.html