Java: Multiple threads vs. sockets

Question

I've written a simple application in Java where there are two nodes, each with a ServerSocket open to a port listening for incoming connections. The nodes run two threads each, sending 1000 messages to the other node through a persistent TCP socket created when sending the first message. However, the nodes do not receive all 1000 messages. One may receive 850 while the other only receives 650. This number tends to stay constant over multiple runs.

The sending code is as follows:

public void SendMsg(String dest, Message myMsg) {
    Socket sendsock = null;
    PrintWriter printwr = null;
    try {
        if(printwr == null) {
            sendsock = new Socket(dest, Main.rcvport);
            printwr = new PrintWriter(sendsock.getOutputStream(), true);
        }
        String msgtosend = myMsg.msgtype.toString() + "=" + Main.myaddy + "=" + myMsg.content + "\n";
        printwr.print(msgtosend);
    } catch (UnknownHostException ex) {
        System.out.println(ex);
        //DO: Terminate or restart
    } catch (IOException ex) {
        System.out.println(ex);
        //DO: Terminate or restart
    }
}

Performance seems to improve if I use buffwr = new BufferedWriter(printwr) as well and use buffwr.write(...) instead of printwr.print(...), though it doesn't seem to be a complete solution for the data loss. There are no exceptions to show that packets weren't delivered, so according to the sender, they were all sent successfully.

On the receiving end, the accepted connection is treated as follows:

BufferedReader inbuff = new BufferedReader(new InputStreamReader(incoming.getInputStream()));

        while(running) {
            String rcvedln = inbuff.readLine();
            if(rcvedln != null) {
                count++;
                System.out.println(count);
            }
        }

Is there an problem with how the readers and writers have been used that could be causing the problem? Thanks.

Answer 1

Are you closing out the PrintWriter to flush the stream?

} finally {
    printwr.close();
    sendsock.close();
}

Answer 2

SendMsg() is creating a new socket every call, so you aren't using a persistent TCP connection. The method isn't closing the socket, either, so you have a lot of open collections. You may be reaching a limit to the number of connections the process can make (the sockets may not be closed when the objects are garbage collected).

Finally, as kd304 pointed out, the Javadoc for PrintWriter states this about the autoFlush parameter of the PrintWriter constructor: "if true, the println, printf, or format methods will flush the output buffer". Your code wasn't calling a method that did a flush.

Try this:

public class MessageSender implements Closeable {
  private final Socket socket;
  private final PrintWriter writer;

  public MessageSender(String dest, int port) {
    socket = new Socket(dest, port);
    writer = new PrintWriter(socket.getOutputStream(), true);
  }

  public void sendMessage(Message message) {
    try {
        writer.println(message.toString());
    } catch (UnknownHostException ex) {
        System.out.println(ex);
        //DO: Terminate or restart
    } catch (IOException ex) {
        System.out.println(ex);
        //DO: Terminate or restart
    }
}

@Override
public void close() throws IOException {
  writer.close();
  socket.close();
}

Note I modified the code so that sendMessage() calls Message.toString() to get the formatted message. It doesn't seem right for sendMessage() to reference fields in Message in order to format the message. Instead of using toString() you could create a method in Message specifically for this purpose.

Here's the server side code:

public class Server implements Runnable {
  private final ServerSocket serverSocket;
  private final ExecutorService executor;
  private volatile boolean running = true;

  public Server(int port, ExecutorService executor) throws IOException {
    serverSocket = new ServerSocket(port);
    this.executor = executor;
  }

  @Override
  public void run() throws IOExeption {
    while (running) {
      Socket socket = serverSocket.accept();
      executor.execute(new ConnectionHandler(socket));
    }
  }

  public boolean stop(long timeout, TimeUnit unit) {
    running = false;
    executor.shutdown();
    return executor.awaitTermination(timeout, unit);
  }
}

You can use Executors to create an ExecutorService to run the tasks. Note that ConnectionHandler needs to close the socket it is given.

Answer 3

Ah, sorry. I accidentally removed the commenting from the code. It's actually like this:

public void SendMsg(String dest, Message myMsg) {
Socket sendsock = null;
try {
    if(printwr == null) {
        sendsock = new Socket(dest, Main.rcvport);
        printwr = new PrintWriter(sendsock.getOutputStream(), true);
    }
    String msgtosend = myMsg.msgtype.toString() + "=" + Main.myaddy + "=" + myMsg.content + "\n";
    printwr.print(msgtosend);
} catch (UnknownHostException ex) {
    System.out.println(ex);
    //DO: Terminate or restart
} catch (IOException ex) {
    System.out.println(ex);
    //DO: Terminate or restart
}

}

printrw is declared and stored outside the function, so once it's set up, there is no need for sendsock or for reinitializing printrw. In the actual application, I'm storing the PrintWriter for every connection in a HashMap and retrieving it at the start of the SendMsg(...) function.

Since the connections are persistent, every time one is accepted, a new thread is lunch that runs a while loop to check it continuously for data. These threads and connections are only closed once the application is terminated. In addition to my previous question, is there a more efficient way of doing this?

Earlier, I'd implemented this code without the "\n" and using println(...) instead and I still had the issue of some messages not being received, so I'm not sure what is causing the problem. The messages are sent like so:

public class SendPortal2 implements Runnable {
String dest = null;

SendPortal2 (String dest) {
    this.dest = dest;
}

public void run() {
        for(int i=1; i<1000; i+=2) {
            Message myMsg = new Message("Message", Main.myaddy + " " + String.valueOf(i));
            Main.myCommMgr.SendMsg(dest, myMsg);
        }
}

}

There are two such threads running. When I ran the code again just now, one side got 999 packets whereas the other one only got 500, leading me to believe sometimes the data from an entire thread could be blocked out. Is that likely?

Thanks for the replies!

Answer 4

If I put a Thread.sleep(2) inside the for loop where the SendMsg function is called, more messages are received properly, but it's not always 1000. Could it be possible that the system's resources are being hogged by two threads running while loops continuously?