Client Sends XML, Server Parses XML, Queues Up and Executes Commands

Question

I recently had a Java interview where I was asked to write a client-server application. I don't code primarily in Java, but with my preliminary Java knowledge, I thought I built a reasonable working solution. After evaluating my solution, I was told that I wasn't going to be interviewed further. I'm fine with their decision, but I'd like to know what parts of my solution weren't right. Any feedback on my code below will be greatly appreciated.

These were the requirements:

Client: Program called ClientEmulator

• Command line option to specify TCP/IP Address, port, and file to read XML data file.

• Should open XML data file and send to server. XML will be of this format

  <Message> <Command>Print</Command> <Command>Display</Command> <Command>Query</Command> </Message> 

• Display the response sent by server to the console.

Server: Program called ServerSocketMonitor

• Command line option to specify TCP/IP Address and port

• Socket Listener: Open socket and listen for incoming data

• Send client a receipt notice when a command is received

• Application must remain operational until CTL+C is detected

• Server will create a command queue and process each client command in a separate thread

• Server's execution/processing of these commands was supposed to be dumb: just print the command name and the execution time

This was my solution:

Client

import java.io.BufferedReader; import java.io.FileReader; import java.io.IOException; import java.io.InputStreamReader; import java.io.PrintWriter; import java.net.Socket; /** * * @author DigitalEye */ public class ClientEmulator { // Singleton Implementation: In the context of this app, private static ClientEmulator _singletonInstance = null; // Default constructor is private to make sure it can't be accessed outside // the class private ClientEmulator() { } /** * Returns single instance of ClientEmulator * @return Instance of ClientEmulator */ public static ClientEmulator getSingleClientEmulator() { if (_singletonInstance == null) { _singletonInstance = new ClientEmulator(); } return _singletonInstance; } /** * Waits on response from server * @param socket Server socket */ public void readServerResponse(Socket socket) { try { BufferedReader serverReader = new BufferedReader( new InputStreamReader(socket.getInputStream())); String serverResponse = null; while ((serverResponse = serverReader.readLine()) != null) { System.out.println("Server Response: " + serverResponse); } } catch (IOException ex) { System.out.println("Error: Unable to read server response\n\t" + ex); } } /** * @param args the command line arguments */ public static void main(String[] args) { // Make sure command line arguments are valid if (args.length < 3) { System.out.println("Error: Please specify host name, port number and data file.\nExiting..."); return; } try { // Open a socket to the server Socket socket = new Socket(args[0], Integer.parseInt(args[1])); // Wait for the server to accept connection before reading the xml file BufferedReader reader = new BufferedReader( new FileReader (args[2])); String line; StringBuilder stringBuilder = new StringBuilder(); while((line = reader.readLine() ) != null) { stringBuilder.append(line); } // Send xml data to server PrintWriter writer = new PrintWriter(socket.getOutputStream(), true); writer.println(stringBuilder.toString()); // Wait for server response getSingleClientEmulator().readServerResponse(socket); } catch (IOException | NumberFormatException ex) { System.out.println("Error: " + ex); } } } 

Server

import java.io.BufferedReader; import java.io.IOException; import java.io.InputStreamReader; import java.io.PrintWriter; import java.io.StringReader; import java.net.InetSocketAddress; import java.net.ServerSocket; import java.net.Socket; import java.net.SocketAddress; import java.text.SimpleDateFormat; import java.util.ArrayList; import java.util.Date; import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; import org.w3c.dom.Document; import org.w3c.dom.Element; import org.w3c.dom.Node; import org.w3c.dom.NodeList; import org.xml.sax.InputSource; import org.xml.sax.SAXException; /** * * @author DigitalEye */ public class SocketServerMonitor { /** * Server Socket */ private ServerSocket _serverSocket = null; /** * Client Socket */ private Socket _clientSocket = null; /** * List of client commands */ private ArrayList<Runnable> _workQueue = null; /** * Date Time formatter */ private final SimpleDateFormat _dateTimeFormatter = new SimpleDateFormat("YYYY/MM/DD HH:mm:ss"); /** * Singleton Implementation: In the context of this app, only one instance of * SocketServerMonitor makes sense */ private static SocketServerMonitor _singletonInstance = null; /** * Hidden constructor prevents instantiation of this type */ private SocketServerMonitor() { } /** * Creates, if the singleton instance hasn't been created before and returns * the single instance of SocketServerMonitor * * @return Singleton Instance of SocketServerMonitor */ public static SocketServerMonitor getSingleSocketServerMonitor() { if (_singletonInstance == null) { _singletonInstance = new SocketServerMonitor(); } return _singletonInstance; } /** * Starts monitoring the server socket. Blocks until a client connects on * this socket * @param serverEndPoint Address to which the server socket is bound */ public void start(SocketAddress serverEndPoint) { if (_serverSocket != null) { stop(); } else { try { _serverSocket = new ServerSocket(); _serverSocket.bind(serverEndPoint); System.out.println("Listening for connections at " + serverEndPoint.toString()); _clientSocket = _serverSocket.accept(); } catch (IOException ex) { System.out.println("Error: Unable to Start Server Socket!\n\t" + ex); } } } /** * Stop monitoring. Closes server and client sockets */ public void stop() { if (_serverSocket != null) { try { System.out.println("Stopping Server Socket Monitor"); _serverSocket.close(); _clientSocket.close(); _serverSocket = null; } catch (IOException ex) { System.out.println("Error: Unable to stop server socket monitor! " + ex); } } } /** * Adds a client command to work queue * @param commandNode Node corresponding to XML element Command */ private void addToWorkQueue(final Node commandNode) { if (_workQueue == null) _workQueue = new ArrayList<>(); _workQueue.add(new Runnable() { @Override public void run() { System.out.println(((Element) commandNode).getTextContent() + " " + _dateTimeFormatter.format(new Date())); } }); } /** * Executes queued client commands. The execution is started in a new thread * so the callee thread isn't blocked until commands are executed. Each command * is executed in a separate thread and the commands are guaranteed to be executed * in the order in which they were received by the server */ private void processCommandQueue() { if (_workQueue != null) { new Thread(new Runnable() { @Override public void run() { try { for (Runnable work : _workQueue) { Thread workerThread = new Thread(work); workerThread.start(); workerThread.join(); } } catch (InterruptedException ex){ System.out.println("Error: Unable to process commands!\n\t" + ex); } finally { _workQueue.clear(); } } }).start(); } } /** * Read a string from the client that contains a XML document hierarchy * @return Document Parsed XML document */ private Document acceptXMLFromClient() { Document xmlDoc = null; try { BufferedReader clientReader = new BufferedReader( new InputStreamReader(_clientSocket.getInputStream())); // block until client sends us a message String clientMessage = clientReader.readLine(); // read xml message InputSource is = new InputSource(new StringReader(clientMessage)); xmlDoc = DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(is); } catch (IOException | ParserConfigurationException | SAXException ex) { // FIXME: put exceptions in log files? System.out.println("Error: Unable to read XML from client!\n\t" + ex); } return xmlDoc; } /** * Waits on client messages. Attempts to read client messages as a XML document * and sends a receipt notice to the client when a message is received. Attempts * to read the Command tag from the client and queues up the client command * for execution */ private void listenToClient() { assert _clientSocket != null : "Invalid Client Socket!"; // open a writer to client socket PrintWriter pw = null; try { pw = new PrintWriter(_clientSocket.getOutputStream(), true); } catch (IOException ex) { System.out.println("Error: Unable to write to client socket!\n\t" + ex); } while (true) { // Read xml from client Document xmlDoc = acceptXMLFromClient(); if (xmlDoc != null) { // Send receipt notice to client if (pw != null) { pw.println("Message Received " + _dateTimeFormatter.format(new Date())); } NodeList messageNodes = xmlDoc.getElementsByTagName("Command"); for (int i = 0; i < messageNodes.getLength(); i++) { addToWorkQueue(messageNodes.item(i)); } } else { System.out.println("Unknown Message Received at " + _dateTimeFormatter.format(new Date())); } processCommandQueue(); } } /** * @param args the command line arguments */ public static void main(final String[] args) { // Make sure command line arguments are valid if (args.length < 2) { System.out.println("Error: Please specify host name and port number.\nExiting..."); return; } try { getSingleSocketServerMonitor().start( new InetSocketAddress(args[0],Integer.parseInt(args[1]))); getSingleSocketServerMonitor().listenToClient(); } catch (NumberFormatException ex) { System.out.println(ex); } } } 

Answer 1

Two of the specific requirements on the server side are:

• server must run until Ctrl-c is pressed.
• it must process commends in a queue in a separate thread.

These two requirements indicate that they want a 'standard' server system, and probably a standard Executor service.

I would expect to see code that looks something like:

ServerSocket serversocket = new ServerSocket(); serversocket.bind(....); ExecutorService threadpool = new Executors.newCachedThreadPool(); while (true) { final Socket client = serversocket.accept(); // will need to create this class that handles the client. // the class should implement Runnable. Runnable clienthandler = new MyClientHandler(client); threadpool.submit(clienthandler); } 

The above code will listen for multiple clients, not just one. It will handle each client in a separate thread.

Update: The question: "My solution was to create a separate command processor thread, which would then span a new thread for each command. Any comments on my approach to ensure that commands are processed in the order that they are received?"

There are two parts to processing the commands... the first part is that they should be acknowledged to the client, and it is appropriate that the ack is in the correct order. There is no requirement that the commands are actually processed in that same order.

Notes:

• *I only just saw that the XML input file was in the question, but 'hidden'... I edited the Q and made it show up properly)
• I never reviewed this part of your problem the first time around.... it is 'bigger' than just the server-socket thread.
• the more I look in to it, the more problems I am seeing..... this is frustrating...
• your server, as it stands, does not notify the client when it receives each command, it just notifies when it receives the first data... and does not print each command....

The network server I described above is pretty typical. Here are some results from google which describe it:

• The Oracle tutorial
• another Oracle tutorial
• A Pattern/Framework for Client/Server Programming in Java
• A multi-threaded socket-based server <-- horrible code style!!!

The point is that they all follow the same pattern, and it's a good one.

Now, your problem is that a client may send more than one request. This is a problem because.... you need a protocol to support it.

• the protocol I imagined in a simple case was that the client 'dumps' the request, and the server processes it, and responds.
• if the client can send multiple requests... how do we know when one request ends, and the next one starts?
• XML input cannot just be read from the socket... because any additional client requests will cause the XML to become invalid....
• your current code just reads a file from disk, and dumps it to the socket.... and does not 'parse' the input in to individual requests. Actually, your code strips all the newlines from the input XML before sending it, so the XML is only one one line.... you should comment that, because it is not obvious.

One of the big 'gotcha's' with network processing is that data does not arrive when you expect it. For example, in your client, it may just hang forever.... because you never close the OutputStream in the client..... in fact.... Your server WILL NOT complete the parsing of the client's data.... --- I just realized that you force all the XML on to one line, and that my previous comment is wrong... you need to **document these things.

I had previously assumed that the protocol was a simple:

• client opens socket
• dumps request on socket's OutputStream
• flushes stream
• closes stream
• listens on socket's InputStream for server response.
• processes responses until InputStream is closed
• done.

But, as 200_success points out, it is nothing of the sort.... you should be forcing the data through the socket, and you are not! You are not flushing the output stream, and you are not closing it.

As a result the timing of when your client request is actually sent is unpredictable.

So, your client-side management is poor.... for a simple single-method client, you have managed to put a lot of broken things in there. I would recommend the following main-method client:

• nevermind, use 200_successes recommendation. It is good.

Given all the above, I would recommend that the socket-server management/configuration is not changed from my previous recommendation, but each client-socket-handling thread in the server should look something like:

class MyClientHandler implements Runnable { // have one thread pool running with as many threads as there are CPU's // this thread pool will process the actual server commands. private static final ExecutorService COMMANDSERVICE = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors()); private final Socket clientSocket; MyClientHandler(Socket client) { clientSocket = client; } public void run() { // parse the XML from the client... try (Writer writer = new OutputStreamWriter(clientSocket.getOutputStream())) { Document document = parseXMLDocument(clientSocket.getInputStream()); NodeList messageNodes = xmlDoc.getElementsByTagName("Command"); for (int i = 0; i < messageNodes.getLength(); i++) { final String command = ((Element)messageNodes.item(i)).getTextContent(); writer.write(String.format("Processing command %d - %s\n", i + 1, command)); writer.flush(); // send the data to the client.... COMMANDSERVICE.submit(new Runnable() { public void run () { // implemented somewhere else. // will print date, and command. processServerCommand(command); } }); } } } 

Answer 2

Protocol

I find your client-server protocol odd. Usually, XML is mostly indifferent to whitespace. That is, a document spread over multiple lines, such as

<hello> <message language="en">How are you</message> </hello> 

… is usually interpreted the same way as

<hello><message language="en">How are you</message></hello> 

That behaviour isn't required by any XML specification, but it is conventional to ignore such whitespace.

However, the protocol you've designed is line-oriented, with each line containing an XML message. The client strips carriage returns / newlines when reading its files. Correspondingly, the server expects the client to send its XML all as one line — it feeds the result of only one readLine() call to the XML parser.

For robustness, the server should be lenient in what it accepts, and that means it should try to read until it sees a complete XML document, possibly spread over multiple lines. Then the client could be simplified — it can just feed the entire input file to the server without stripping carriage returns / newlines.

Answer 3

I should preface this review by noting that it is easy for me to criticize your code, with the luxury of being able to look up whatever resources I want and without the pressure or time constraints of being in an interview.

Client

Why is the class called ClientEmulator — what is it emulating? In fact, what is its purpose? It looks like the only thing in the class is readServerResponse(). Oddly, main() is responsible for sending the request. I would expect that your object would be responsible for sending the request and handling the response. In short, that's not object-oriented programming; it's a function that has been awkwardly incorporated into a class.

Should ClientEmulator really be a singleton? I know that as you wrote it, your main() happens to use just one ClientEmulator. However, most client-server systems allow multiple clients, and multiple clients in the same process wouldn't be inconceivable.

Even if ClientEmulator were a singleton, why instantiate it lazily? The constructor is so trivial. You might as well say

private static ClientEmulator instance = new ClientEmulator(); 

and be done with it. Also, it's customary to name the singleton-fetching function getInstance().

Assuming that you accept my suggested protocol improvements, you could simplify the file I/O.
Just use java.nio.file.Files.copy(Path, OutputStream).

Normal output should go to System.out; error messages should go to System.err to avoid mingling with the output.

After sending the request, it's polite to cleanup. You should at minimum flush() the output stream. In the case of this one-shot client, it's never going to send anything again, so you could just shutdownOutput(). After everything is done, it's good practice to close() the socket, or better yet, use try-with-resources and autoclosing.

import java.io.BufferedReader; import java.io.Closeable; import java.io.IOException; import java.io.InputStreamReader; import java.nio.file.Files; import java.nio.file.FileSystems; import java.nio.file.Path; import java.net.Socket; public class Client implements Closeable { private Socket socket; public Client(String host, int port) throws IOException { this(new Socket(host, port)); } public Client(Socket s) { this.socket = s; } @Override public void close() throws IOException { this.socket.close(); } public void send(String filePath) throws IOException { Path path = FileSystems.getDefault().getPath(filePath); Files.copy(path, this.socket.getOutputStream()); this.socket.shutdownOutput(); } public void recv() { try { BufferedReader serverReader = new BufferedReader( new InputStreamReader(socket.getInputStream())); String serverResponse; while ((serverResponse = serverReader.readLine()) != null) { System.out.println("Server Response: " + serverResponse); } } catch (IOException ex) { System.err.println("Error: Unable to read server response\n\t" + ex); } } public static void main(String[] args) { // Make sure command line arguments are valid String hostname, filePath; int port; try { hostname = args[0]; port = Integer.parseInt(args[1]); filePath = args[2]; } catch (ArrayIndexOutOfBoundsException | NumberFormatException ex) { System.err.println("Error: Please specify host name, port number and data file.\nExiting..."); return; } try (Client c = new Client(hostname, port)) { c.send(filePath); c.recv(); } catch (IOException ex) { System.err.println("Error: " + ex); } } }