Doc Martin Doc Martin - 3 months ago 6
Java Question

The read from a TCP Server hangs when reading more than once

I am testing some TCP code and it seems to work fine except for one problem. The read from the socket hangs in one of the methods when there is nothing more to read:

Here is the TCP code:

package com.comp424.service;

import java.net.InetAddress;
import java.net.ServerSocket;
import java.net.Socket;
import java.net.UnknownHostException;
import java.io.IOException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

public class TCPService implements Runnable
{
protected int serverPort;
protected InetAddress bindAddress;

protected ServerSocket serverSocket = null;
protected boolean isStopped = false;
protected Thread runningThread = null;

protected ExecutorService threadPool = Executors.newFixedThreadPool(10);

public TCPService(String host,int port)
{
serverPort = port;

try
{
bindAddress = InetAddress.getByName(host);
}
catch (UnknownHostException e)
{
throw new RuntimeException("Failed to get bind address", e);
}
}

private void start()
{
try
{
serverSocket = new ServerSocket(serverPort, 10, bindAddress);
}
catch (IOException e)
{
throw new RuntimeException("Cannot open port " + serverPort, e);
}
}

public void run()
{
synchronized (this)
{
runningThread = Thread.currentThread();
}

start();

while (!isStopped())
{
Socket clientSocket = null;

try
{
clientSocket = serverSocket.accept();
}
catch (IOException e)
{
if (isStopped())
{
System.out.println("Server Stopped.");
break;
}
throw new RuntimeException("Error accepting client connection", e);
}

threadPool.execute(new ClientHandler(clientSocket));
}
threadPool.shutdown();

System.out.println("Server Stopped.");
}

public synchronized void stop()
{
isStopped = true;

try
{
serverSocket.close();
}
catch (IOException e)
{
throw new RuntimeException("Error closing server", e);
}
}

private synchronized boolean isStopped()
{
return isStopped;
}
}

package com.comp424.service;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.OutputStream;
import java.net.Socket;
import java.util.ArrayList;
import java.util.List;
import java.util.StringTokenizer;

import com.comp424.impl.dao.DaoFactory;
import com.comp424.intf.dao.ICourseDao;
import com.comp424.intf.dao.IPersonDao;
import com.comp424.intf.dao.IRegisterCourseDao;
import com.comp424.model.Course;
import com.comp424.model.Person;

public class ClientHandler implements Runnable
{
private static IRegisterCourseDao registrationDao;
private static IPersonDao personDao;
private static ICourseDao courseDao;

protected Socket clientSocket = null;

public ClientHandler(Socket socket)
{
registrationDao = DaoFactory.getInstance().getCourseRegistrationDao();
personDao = DaoFactory.getInstance().getPersonDao();
courseDao = DaoFactory.getInstance().getCourseDao();
clientSocket = socket;
}

public void run()
{
try
{
String command = null;

OutputStream output = clientSocket.getOutputStream();
BufferedReader buffer = new BufferedReader(new InputStreamReader(clientSocket.getInputStream()));

command = buffer.readLine();

while (command != null)
{
String separator = ":";

StringTokenizer tokenizer = new StringTokenizer(command, separator);

List<String> tokens = new ArrayList<>();

while (tokenizer.hasMoreElements())
{
tokens.add((String) tokenizer.nextElement());
}

int operation = Integer.parseInt(tokens.get(0));

switch (operation)
{
case 1:
try
{
Person person = personDao.findByID(Long.parseLong(tokens.get(1)));
Course course = courseDao.findByID(Long.parseLong(tokens.get(2)));

registrationDao.register(person, course);
output.write(("0\r\n").getBytes());
}
catch (Exception e)
{
e.printStackTrace();
output.write(("1\r\n").getBytes());
}
break;

case 2:
try
{
Person person = personDao.findByID(Long.parseLong(tokens.get(1)));
Course course = courseDao.findByID(Long.parseLong(tokens.get(2)));

registrationDao.register(person, course);
output.write(("0\r\n").getBytes());
}
catch (Exception e)
{
e.printStackTrace();
output.write(("1\r\n").getBytes());
}
break;

case 3:
try
{
Person person = personDao.findByID(Long.parseLong(tokens.get(1)));

List<Course> courses = registrationDao.findByPerson(person);

for (Course c : courses)
{
output.write((c.getName() + "\r\n").getBytes());
}
}
catch (Exception e)
{
e.printStackTrace();
output.write(("1\r\n").getBytes());
}
break;

}
command = buffer.readLine();
}

output.close();
}
catch (IOException e)
{
// report exception somewhere.
e.printStackTrace();
}
}
}


And here is the code where it just hangs in findRegisteredCourses() after reading two strings returned instead of exiting the while loop:

while (response != null)
{
result.add(response);
System.out.println("findRegisteredCourses():Response = " + response);
response = reader.readLine();

}


Full code for findRegisteredCourses():

@Override
public List<String> findRegisteredCourses(String personID) throws Exception
{
try (Socket server = new Socket("localhost", 7000))
{
List<String> result = new ArrayList<>();

DataOutputStream writer = new DataOutputStream(server.getOutputStream());
BufferedReader reader = new BufferedReader(new InputStreamReader(server.getInputStream()));

String operation = "3:" + personID + "\r\n";
writer.writeBytes(operation);
writer.flush();

String response = reader.readLine();

while (response != null)
{
result.add(response);
System.out.println("findRegisteredCourses():Response = " + response);
response = reader.readLine();

}
server.close();
return result;
}
}

Answer

You're continuing to try to read from the server until it's closed the socket - whereas the server is waiting for another command from the client. Neither side is going to do anything, as they're waiting for the other.

Basically, you need to change your protocol, either to have some "here's the end of the response" indication (such as an empty line, if that's not a valid value in the response data), or to only have a single request/response per connection.

Your suggested "fix" of using the ready() method is very broken - it basically means you assume there's no more data as soon as there's a pause. Maybe the server is taking a while to find the next item. Maybe there's a delay on the network - or maybe it's finished. You can't tell, and basically you're violating the design of streaming protocols (such as TCP) by trying to infer information from the fact that there's no data available right now. Don't do that - fix your protocol.

Comments