“Simple” Network Connections with Java – a problem

I've been playing with NIO for network stuff to see if/how I want to update the code for Head First Java 3rd Edition. The original code used Sockets and Readers/Writers, and I thought there must be a more "modern" way to do this so I've tried updating it a few different ways. I got it working using SocketChannel and ByteBuffer (eventually!), but the ByteBuffer stuff is just too fiddly to use to teach people basic network IO in Java. After turning to Twitter for the answers (Twitter Driven Development) I had what looked like a promising hybrid approach.

As an #OpenJDK NIO Contributor I can tell you that NIO, even without async, is never simple. In most cases, using Channels.* factory methods for streams should be sufficient for most cases -- unless you write heavily I/O-bound code. They allow you to do NIO but it feels like IO.

— ☕ Head Crashing Informatics | Party On! 🤘 (@mkarg) December 20, 2021

However, I've run into a problem with using the Channels factory methods to provide this nice bridge between NIO (SocketChannels) and classic streaming IO.

I'm posting the code here because you can't post source code this long on Twitter! Maybe someone can help me.

The original code from Head First Java 2nd Edition:

package ch15;

import java.io.BufferedReader;
import java.io.InputStreamReader;
import java.io.PrintWriter;
import java.net.ServerSocket;
import java.net.Socket;
import java.util.ArrayList;

public class SimpleChatServer {
  ArrayList<PrintWriter> clientOutputStreams;

  public static void main(String[] args) {
    new SimpleChatServer().go();
  }

  public void go() {
    clientOutputStreams = new ArrayList<>();
    try {
      ServerSocket serverSock = new ServerSocket(5000);
      while (true) {
        Socket clientSocket = serverSock.accept();
        PrintWriter writer = new PrintWriter(clientSocket.getOutputStream());
        clientOutputStreams.add(writer);
        Thread t = new Thread(new ClientHandler(clientSocket));
        t.start();
        System.out.println("got a connection");
      }
    } catch (Exception ex) {
      ex.printStackTrace();
    }
  } // close go

  public void tellEveryone(String message) {
    for (PrintWriter writer : clientOutputStreams) {
      try {
        writer.println(message);
        writer.flush();
      } catch (Exception ex) {
        ex.printStackTrace();
      }
    } // end while
  } // close tellEveryone

  public class ClientHandler implements Runnable {
    BufferedReader reader;
    Socket socket;

    public ClientHandler(Socket clientSocket) {
      try {
        socket = clientSocket;
        InputStreamReader isReader = new InputStreamReader(socket.getInputStream());
        reader = new BufferedReader(isReader);
      } catch (Exception ex) {
        ex.printStackTrace();
      }
    } // close constructor

    public void run() {
      String message;
      try {
        while ((message = reader.readLine()) != null) {
          System.out.println("read " + message);
          tellEveryone(message);
        } // close while
      } catch (Exception ex) {
        ex.printStackTrace();
      }
    } // close run
  } // close inner class
} // close class

Code in gist

When I say this works, I mean you can launch one or more chat clients (code at the end of this blog) to talk to the server, and:

1. The server will see the message
2. The client and all other clients will see that message echoed back to them.

I wrote a simple test harness to check this, it's not a real automated test but it's a faster way to poke the server than the client.

package ch15;

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.PrintWriter;
import java.net.Socket;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

import static org.junit.jupiter.api.Assertions.assertTrue;

// Needs a SimpleChatServer running (is not a real unit test class, more like an integration test)
class SimpleChatServerTest {
  @Test
  void testSend() throws IOException {
    Socket socket = connect();
    assertTrue(send(socket, "Hi"));
    socket.close();
  }

  @Test
    // this will work if run when there are no other clients connected
    // this will likely hang if other clients are already connected (e.g. running testReceive)
  void testSendAndReceive() throws IOException, InterruptedException {
    Socket socket = connect();
    ExecutorService executorService = Executors.newSingleThreadExecutor();
    executorService.submit(() -> Assertions.assertEquals("Hi", receive(socket)));
    assertTrue(send(socket, "Hi"));
    Thread.sleep(10_000);
    socket.close();
  }

  @Test
    // NOT a real unit test - run this to have a client sat waiting for a response. You can get this to
    // pass by sending "Hi" to the server with another client, e.g. by calling testSend.
  void testReceive() throws IOException {
    Socket socket = connect();
    Assertions.assertEquals("Hi", receive(socket));
    socket.close();
  }

  public boolean send(Socket socket, String payload) throws IOException {
    PrintWriter writer = new PrintWriter(socket.getOutputStream());

    writer.println(payload);
    writer.flush();
    return true;
  }

  public String receive(Socket socket) {
    try (InputStreamReader streamReader = new InputStreamReader(socket.getInputStream());
         BufferedReader reader = new BufferedReader(streamReader)) {
      String message = reader.readLine();
      System.out.println("read " + message);
      return message;

    } catch (IOException e) {
      e.printStackTrace();
    }
    return null;
  }

  private Socket connect() throws IOException {
    Socket socket = new Socket("127.0.0.1", 5000);
    System.out.println(socket.getLocalPort() + " connected");
    return socket;
  }

}

Code in gist

Make sure SimpleChatServer is running when you run these tests. Several ways to poke the server to see it works:

1. Run testSend, the test passes and you should see the message logged on the server console output.
2. Run testSendAndReceive on its own, with no other clients connected. It should pass, and you should see the message logged on the server and the response logged in the test's output console too.
3. Run testReceive - this will sit and spin waiting for a response. Run testSend, and you should see that both testSend and testReceive pass. This tests that two clients can be connected and that they both receive the same message if one client sends it.

So this works. However, if I convert the code to use SocketChannels and the Channels factory methods (or if indeed I get the Socket directly from the SocketChannel), I have some sort of contention/blocking/deadlock.

package ch15;

import java.io.BufferedReader;
import java.io.InputStreamReader;
import java.io.PrintWriter;
import java.net.InetSocketAddress;
import java.nio.channels.Channels;
import java.nio.channels.ServerSocketChannel;
import java.nio.channels.SocketChannel;
import java.util.ArrayList;

public class SimpleChatServer {
  ArrayList<PrintWriter> clientOutputStreams;

  public static void main(String[] args) {
    new SimpleChatServer().go();
  }

  public void go() {
    clientOutputStreams = new ArrayList<>();
    try {
      ServerSocketChannel serverSocketChannel = ServerSocketChannel.open();
      serverSocketChannel.bind(new InetSocketAddress("localhost", 5000));

      while (true) {
        SocketChannel clientSocket = serverSocketChannel.accept();
        PrintWriter writer = new PrintWriter(Channels.newOutputStream(clientSocket));
        clientOutputStreams.add(writer);
        Thread t = new Thread(new ClientHandler(clientSocket));
        t.start();
        System.out.println("got a connection");
      }
    } catch (Exception ex) {
      ex.printStackTrace();
    }
  } // close go

  public void tellEveryone(String message) {
    for (PrintWriter writer : clientOutputStreams) {
      try {
        writer.println(message);
        writer.flush();
      } catch (Exception ex) {
        ex.printStackTrace();
      }
    } // end while
  } // close tellEveryone

  public class ClientHandler implements Runnable {
    BufferedReader reader;
    SocketChannel socket;

    public ClientHandler(SocketChannel clientSocket) {
      try {
        socket = clientSocket;
        InputStreamReader isReader = new InputStreamReader(Channels.newInputStream(socket));
        reader = new BufferedReader(isReader);
      } catch (Exception ex) {
        ex.printStackTrace();
      }
    } // close constructor

    public void run() {
      String message;
      try {
        while ((message = reader.readLine()) != null) {
          System.out.println("read " + message);
          tellEveryone(message);
        } // close while
      } catch (Exception ex) {
        ex.printStackTrace();
      }
    } // close run
  } // close inner class
} // close class

Code in gist

Code changes are on lines 20-23 and 47-52.

Now running this chat server and poking it with the test harness:

1. Running testSend, the test passes and all works as expected - yay!
2. Running testSendAndReceive on its own, with no other clients connected passes - yay!
3. Run testReceive - this will sit and spin waiting for a response. Run testSend (which will pass) but testReceive won't receive anything, it just sits there waiting forever....

Except... when you start blogging about the problem and you try to reproduce it, and it all works as you expect. What. The. F...? Oh yeah. Don't forget to restart SimpleChatServer with the new code, sigh.

Debugging the issue appears to show the calls to the writer in tellEveryone are blocked. There's a wait on a lock in those methods, so I assume that one of the other threads (each ClientHandler is running on its own thread) has grabbed some resource which is needed to perform the write. Since the code works in its earlier form, I assume the problem is not specifically the PrintWriter, but something to do with the channels/byte buffers under the covers.

I also wonder if I could delegate the writing to the thread which is doing the reading, if that would solve the problem. This solution at the moment has the server spinning up one thread per client connected to it, and that thread is responsible for reading all the client's messages and writing that message to ALL clients. Which seems... like it might not be the best approach.

Also, yes I did try a) switching the ArrayList to a CopyOnWriteArrayList (because it's shared by multiple threads). It's written to by a single thread though so it's not super terrible as it is (although yes, I did get a concurrent modification once). And b) I also tried adding synchronized to the tellEveryone method, because
everyone knows the way to fix multithreading issues is to stick synchonized on everything...

Anyway. If anyone can let me know what's going on here and/or fix my problem, that would be amazing 🙂 If not... at least this has been an interesting exercise in learning more about NIO, and learning yet more about concurrency issues, sigh....

PS As promised, simple client code (this is the code from Head First Java 2nd Edition):

package ch15;

import javax.swing.*;
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;

public class SimpleChatClient {
  JTextArea incoming;
  JTextField outgoing;
  BufferedReader reader;
  PrintWriter writer;
  Socket sock;

  public static void main(String[] args) {
    SimpleChatClient client = new SimpleChatClient();
    client.go();
  }

  public void go() {
    JFrame frame = new JFrame("Ludicrously Simple Chat Client");
    JPanel mainPanel = new JPanel();
    incoming = new JTextArea(15, 50);
    incoming.setLineWrap(true);
    incoming.setWrapStyleWord(true);
    incoming.setEditable(false);
    JScrollPane qScroller = new JScrollPane(incoming);
    qScroller.setVerticalScrollBarPolicy(ScrollPaneConstants.VERTICAL_SCROLLBAR_ALWAYS);
    qScroller.setHorizontalScrollBarPolicy(ScrollPaneConstants.HORIZONTAL_SCROLLBAR_NEVER);
    outgoing = new JTextField(20);
    JButton sendButton = new JButton("Send");
    sendButton.addActionListener(new SendButtonListener());
    mainPanel.add(qScroller);
    mainPanel.add(outgoing);
    mainPanel.add(sendButton);
    setUpNetworking();
    Thread readerThread = new Thread(new IncomingReader());
    readerThread.start();
    frame.getContentPane().add(BorderLayout.CENTER, mainPanel);
    frame.setSize(800, 500);
    frame.setVisible(true);
  } // close go

  private void setUpNetworking() {
    try {
      sock = new Socket("127.0.0.1", 5000);
      InputStreamReader streamReader = new InputStreamReader(sock.getInputStream());
      reader = new BufferedReader(streamReader);
      writer = new PrintWriter(sock.getOutputStream());
      System.out.println("networking established");
    } catch (IOException ex) {
      ex.printStackTrace();
    }
  } // close setUpNetworking

  public class SendButtonListener implements ActionListener {
    public void actionPerformed(ActionEvent ev) {
      try {
        writer.println(outgoing.getText());
        writer.flush();
      } catch (Exception ex) {
        ex.printStackTrace();
      }
      outgoing.setText("");
      outgoing.requestFocus();
    }
  } // close inner class

  public class IncomingReader implements Runnable {
    public void run() {
      String message;
      try {
        while ((message = reader.readLine()) != null) {
          System.out.println("read " + message);
          incoming.append(message + "\n");
        } // close while
      } catch (Exception ex) {
        ex.printStackTrace();
      }
    } // close run
  } // close inner class
} // close outer class

Code in gist

Note that you can see the problem by running more than one chat client and trying to talk to the server. You should see the messages from one client echoed back to itself and all other clients.

I also just noticed an interesting thing using this 2nd edition client code with the SocketChannel server (I was running two clients): If you send a message from one client (client1), you can see it logged to the server console, but it does not come back to any of the clients. If you then send a message from the other client (client2), client2 then sees the first message, and the second message, and client1 sees the first message. If you then send a message from client1, you get some more messages back at both clients (but not all the messages). Basically if you swap between the clients it seems to unblock it. To me that suggests whatever's reading a client's input is also causing the blockage to the writing.

PPS here's the code for the client and server with NIO & ByteBuffers.

PPPS Pull request (not mine!)