Thread: Our client-server miniprogram is not communicating right, blocking readLine does not block?

Greetings!

Me and my brother are working on a small java project to learn network programming and get some experience working with networking code. We decided to make a relatively simple client/server program where clients get into a simple chat lobby and can invite others to play small games(like pong). The basic server code has been written, and we have made a special client that is used for debugging.

We use a self-defined protocol of Strings where a message looks like "4-|user,pass". The number before the delimiter "-|" is the operation code, that tells the server what kind of message this client sends. Based on that number, the server dispatches the message to the appropriate handler method. 4 is authentication for example, and the handler looks the user and pass up in a file and if found, returns true, otherwise, false. Then the server responds to the clinet with 2-|"any message" where the client will recognize opcode 2 as a "authentication accepted" and proceed with the next part of client code. In a similar way, we plan to write all message types(both in the game, in the lobby and in a game setup room).

While testing we ran into a problem where the BufferedReader .readLine() does not seem to be a blocking call like it should be, so the client keeps spamming 'null' in the output field that we made to see the server response to the message we send. When we try to debug the server code and set breakpoints at the suspicious locations, it strangely skips both while(true) loops without activating either breakpoint and executes the finally{} code, even though the client did not close the connection and the second while loop was never entered. The first while loop IS entered though, because the test client gets a "0" on its output, which is the server message indicating "please authenticate yourself".

Also, if we approach things in a dumb way that can be done way more efficiently or easier to read/manage, please do tell. Keep in mind we are beginners though! We decided to use messages in a string format and decode it at both sides as it seemed easier than transmitting java objects and making sure they are of the same type, also for reducing overhead as much of possible.

http://pastebin.com/embed_js.php?i=UY4iP3p3
http://pastebin.com/embed_js.php?i=8XY3kuy5

The server code

package Chatroom;
 
import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.PrintWriter;
import java.net.ServerSocket;
import java.net.Socket;
import java.util.HashSet;
 
/**
 * This is the main server class. It is the core of the server functionality, and the listener for incoming clients that will
 * spawn threads to deal with clients, track their information, change their states and ultimately, clean up after all clients by keeping the
 * data structures up to date.
 * @version 0.1
 *
 */
public class Server {
	public static final int PORT = 9001; //fixed port to be used by the server for now
	/**
	 * The set of all names of clients in the main room.
	 */
	private static HashSet<String> names = new HashSet<String>();
 
	/**
	 * The set of all the print writers for all the clients. This is kept so we can easily broadcast messages.
	 */
	private static HashSet<PrintWriter> writers = new HashSet<PrintWriter>();
 
	/**
	 * The application's main method, which listens on the specified port and spawns handler threads
	 */
	public static void main(String[] args) throws Exception {
		ServerLogger.log("Server started.");
		ServerSocket listener = new ServerSocket(PORT);
		try {
			while (true) {
				new Handler(listener.accept()).start();
			}
		} finally {
			listener.close();
		}
	}
 
	/**
	 * A handler thread class. Handlers are spawned from the listening loop and are responsible for
	 * dealing with a single client and broadcasting its messages.
	 */
	private static class Handler extends Thread {
		private String name; //Holds the username of this client only after authenticating.
		private Socket socket;
		private BufferedReader in;
		private PrintWriter out;
		public User user;
		private UserState state; //The state that the user is currently in. This is used for masking certain actions, and only allow those actions appropriate for
		//this user state. For example a user in a game cannot accept invitations by other players. A user in the lobby cannot make a player move command.
 
		/**
		 * Constructs a handler thread. All the interesting work is done in the run method.
		 */
		public Handler(Socket socket) {
			this.socket = socket; //the supplied Socket is the one made when the listener accepted a client.
		}
 
		public void run() {
			try {
				//Create character streams for the socket.
				in = new BufferedReader(new InputStreamReader(socket.getInputStream()));
				out = new PrintWriter(socket.getOutputStream(), true);
 
				//First, authenticate user and keep his user details.
				while(true) {
					out.println( "" + OpCodeSC.AUTHENTICATE.ordinal()); //Ask for authentication
					String response = in.readLine(); //Read client response (should contain user and pass). User can disconnect at this point.
					int opCode = Integer.parseInt(response.split("-|")[0]);
					String subString = response.split("-|")[1];
					if (opCode == OpCodeCS.AUTHENTICATE.ordinal() && MessageHandler.dispatch(opCode, subString, null) == true){
						out.println("Authentication succesful.");
						break;
					}
					else {
						out.println("Authentication not succesful.");
					}
					//TODO: Fix to ensure proper user states and corresponding masking vectors for allowed actions.
				}
 
				//Second, keep accepting client messages, handling them, and sending responses.
 
				while(true){
					String response = in.readLine();
					//Dissasemble and handle all client input.
					int opcode = Integer.parseInt(response.split("-|")[0]);
					String subString = response.split("-|")[1];
					MessageHandler.dispatch(opcode, subString, null);
				}
 
			} catch (IOException e) {
				ServerLogger.log(e.toString());
			} finally {
				//This client is going down. Remove its name and print writer from the sets, and close its socket.
				if (name != null) {
					names.remove(name);
				}
				if (out != null) {
					writers.remove(out);
				}
				ServerLogger.log("Client closed connection.");
				try {
					socket.close();
				} catch (IOException e) {
					ServerLogger.log("Failed closing socket for a client that disconnected");
				}
			}
 
 
		}
	}
}

The special debugging Client:

package Chatroom;
 
import java.awt.*;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.PrintWriter;
import java.net.Socket;
 
import javax.swing.*; 
 
/*
 * This special client is made for debugging purposes. It allows to enter a custom message and send it as is to the server. By following the
 * communication protocol, it is possible to simulate every client action and verify that the server correctly processes the response.
 * @version 0.1
 */
public class AdminClient {
 
	BufferedReader in;
	PrintWriter out;
	JFrame frame = new JFrame("Chatter");
	JTextField textField = new JTextField(40);
	JTextArea messageArea = new JTextArea(8, 40);
 
	/**
	 * Constructs the client by laying out the GUI and registering a listener with the
	 * textfield so that pressing Enter in the listener sends the textfield contents
	 * to the server.
	 */
	public AdminClient() {
 
		//Layout GUI
		textField.setEditable(true);
		messageArea.setEditable(false);
		frame.getContentPane().add(textField, "North");
		frame.getContentPane().add(new JScrollPane(messageArea), "Center");
		frame.pack();
 
		//Add listeners
		textField.addActionListener(new ActionListener() {
			public void actionPerformed(ActionEvent e) { //When pressed ENTER in the textfield, this is triggered
				out.println(textField.getText());
				textField.setText("");
			}
		});
	}
 
	/**
	 * Ask for the server IP address that the user wants to connect to
	 */
	private String getServerAddress() {
		return JOptionPane.showInputDialog(frame, "Enter IP of server:", "Welcome to the chatbox",
				JOptionPane.QUESTION_MESSAGE);
	}
 
	/**
	 * Ask for the desired username.
	 */
	private String getName() {
		return JOptionPane.showInputDialog(frame, "Choose a screen name:", "Username required",
				JOptionPane.PLAIN_MESSAGE);
	}
 
	/**
	 * Connects to the server then enters the processing loop.
	 */
	public void run() throws IOException {
		String serverAddress = getServerAddress();
		Socket socket = new Socket(serverAddress, 9001);
		in = new BufferedReader(new InputStreamReader(socket.getInputStream()));
		out = new PrintWriter(socket.getOutputStream(), true);
 
		//Process all server messages, according to the protocol
		while (true) {
			String line = in.readLine();
				messageArea.append(line + "\n"); //Start message at character 8(after the protocol word MESSAGE)
		}
	}
 
	 public static void main(String[] args) throws Exception {
			AdminClient client = new AdminClient();
			client.frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
			client.frame.setVisible(true);
			client.run();
 
		 } 
 
 
}

The OpCodeCS, a basic enumeration to prevent ourselves from using fixed number codes for operations

package Chatroom;
 
/**
* Collects all Opcodes from clients to the server.
* @version 0.1
*/
 
public enum OpCodeCS {
	CONNECT,CHATMESSAGE,COMMAND,DISCONNECT,AUTHENTICATE;
}

MessageHandler

package Chatroom;
 
/**
 * Receive client messages given to the server and dispatch these to the proper handler methods.
 * @version 0.1
 */
public class MessageHandler {
	/*
	* The return value indicates whether the message was succesfully handled (and accepted), or denied.
	* True = accepted, False = denied. There is currently no additional information supplied.
	*/
	public static Boolean dispatch(int opcode, String msg, User user) {
		try{
			switch (OpCodeCS.values()[opcode]) {
			case CONNECT: //connect.
			case CHATMESSAGE: //chatmessage.
			case COMMAND: //chatcommand.
			case DISCONNECT: //disconnect.
			case AUTHENTICATE: //authenticate.
				authenticate(msg);
			default: return true;
			}	
		}
		catch (ArrayIndexOutOfBoundsException exception){
			ServerLogger.log(exception.toString());
		}
		return false;
	}
 
	private static boolean authenticate(String msg){
		String userName = msg.split(",")[0];
		String pass = msg.split(",")[1];
		if (Authentication.IsaMatch(userName, pass) == true)
			return true;
		else
			return false;
	}
 
 
	/*
	 * chatmessage voorbeeld:
	 * dispatch( OpCodeCS.CHATMESSAGE.ordinal(), "Hey guys!", Tom);
	 * Goal:  Tom stuurt een boodschap in de chat. Elke user die momenteel als state "INLOBBY" heeft, moet deze boodschap krijgen.
	 * 			De printwriter van elke User met User.state==INLOBBY moet de boodschap (Tom.name + ": " + "Hey Guys!") krijgen.
	 */
}

ServerLogger:

package Chatroom;
 
import java.text.SimpleDateFormat;
import java.util.Calendar;
 
/**
 * Log all server activity using this server logger. Each piece of information that should be logged on the server should use an instance of this class
 * and call its .log method, with as the argument the message to be logged. The timestamp will be added automatically, therefore the message should not 
 * include this.
 * Ultimately, this class will provide functionality to specify whether a GUI is to be used or not, and if so, it will redirect the messages to the appropriate
 * GUI element. When disabled, the server logs are written to the console where the server runs. File storing functionality will be provided as well.
 * @version 0.1
 *
 */
public class ServerLogger {
 
	public ServerLogger(){
 
	}
 
	public static void log(String string){
		String timeStamp = new SimpleDateFormat("yyyyMMdd_HHmmss").format(Calendar.getInstance().getTime());
		System.out.print(timeStamp + ": " + string + "\n");
	}
}

User:

package Chatroom;
 
import java.io.PrintWriter;
 
/**
 * Every user that is currently connected has his/her own instance of the User class. This holds volatile data that is currently associated with this user.
 * @version 0.1
 */
public class User {
 
	/*
	 * The state this user is currently in. This is used to determine the possible operations this user can perform, and block the illegal ones.
	 */
	int state;
	/*
	 * Every user has an associated writer that is used to send messages to that user. When broadcasting, the user list will be iterated, and every user in a 
	 * matching state will be sent the message. Can also be used to sending invitations to the appropriate user.
	 */
	PrintWriter out;
	String name;
 
	public User(String username, PrintWriter writer) {
		this.state = UserState.LOBBY.ordinal(); //Default state when a user instance is created(after authenticating)
		this.name = username;
		this.out = writer; //The output stream to direct messages to this user to.
	}
}

OpCodeSC (SC means Server to Client)

package Chatroom;
 
/**
* Collects all Opcodes from the server to clients.
* @version 0.1
*/
 
public enum OpCodeSC{
	AUTHENTICATE;
}

and finally, Authentication:

package Chatroom;
 
import java.io.BufferedReader;
import java.io.FileReader;
import java.util.HashMap;
import java.util.Map;
 
/**
 * Static methods to check and execute authentication of users.
 * @version 0.1
 */
public class Authentication {
 
	/*
	 * The location of the user/passwords file.
	 */
	private final static String PATH="D:/Working/workspace/Java networking testing/src/Chatroom/userdata.txt";
	static Map<String, String> userdata;
 
	public static void initialize() {
		try {
			String next;
			userdata = new HashMap<String, String>();
			FileReader fr = new FileReader(PATH);
			BufferedReader br = new BufferedReader(fr);
			while ((next = br.readLine()) != null) {
				//Disassemble next into a user/pass combination and put the results in a HashMap
				userdata.put(next.split(",")[0], next.split(",")[1]);
			}
			br.close();
		} catch (Exception e) {
 
		}
	}
 
	/*
	 * Enforce the username policy by only accepting valid usernames. When the policy changes, this is the implementation that will change.
	 * A valid username is currently one that has between 3 and 20 characters, and contains only lower- and uppercase letters and numbers and underscores.
	 */
	public static Boolean isValidUsername(String user) {
		return true;
	}
 
	public static Boolean isValidPass(String pass) {
		if (pass.length() >= 4 && pass.length() <= 20)
			return true;
		return false;
	}
 
	/*
	 * @Return	True if and only if the given password matches with the given username, and is an existing record in the underlying storage system.
	 */
	public static Boolean IsaMatch(String user, String pass) {
		initialize();
		if (userdata.containsKey(user) && userdata.get(user) == pass)
			return true;
		else
			return false;
	}
 
	/*
	 * Add the user with the accompanying password to the underlying storage system.
	 */
	public static void addUser(String user, String pass) {
 
	}
 
	/*
	 * Find the record with the given username, and delete the user from the underlying storage system.
	 */
	public static void deleteUser(String user) {
 
	}
 
 
}

We have solved the problem.
Our delimiter "-|" was a regular expression and interpreted as - OR empty string, then parsing an empty string into a number gave an exception.
Meanwhile I also learned that the null spamming comes from the fact that when a socket is closed on the other side, readline() is not a blocking method anymore and I included a check for if (line == null) close the socket on this end too.

We changed the delimiter for @> (another arbitrary combination).