“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.

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!)

4 Replies to ““Simple” Network Connections with Java – a problem”

    1. Someone who has to tell someone else that they’re not a true software developer clearly has issues with their own confidence. It’s OK. Not knowing the answers and asking for help is completely fine. If you work somewhere where it’s not fine, RUN AWAY. We need more collaboration and empathy in our industry, not less.

  1. Looking at the wrapper steam class used by the Channels static method I found that as SocketChannel extends SelectableChannel, the actual writing and reading done on the channel is inside a synchronized block using the SelectableChannel.blockingLock() as lock.

    ChannelInputStream: https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java#L61
    ChannelOutputStream: https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/nio/ch/ChannelOutputStream.java#L76

    So I think the ClientHandler threads are holding the lock for their respective connection while waiting on the blocking read. When tellEveryone is called, it will try to get the lock for each connection which will be provided only after the corresponding connection is not waiting on a read. So it will be able to immediately write on the connection the message has been received from, but for the others it will need to wait for them to have received some message.

    I think it match the behavior you experimented.

    The synchronization block is there, I think, to protect from the a change of the blocking behavior while the configuration is check and the read or write are preformed.

    It makes the read/write calls sequential, which for a simple ReadableByteChannel or WriteableByteChannel would be an expected behavior. But for the case of the SocketChannel that is both at the same time, it makes both read and write (if using the stream wrapper) sequential. As they are blocking operation, it is not the expected (and probably not the most efficient also) behavior of a socket connection.

    I don’t see how keeping this sequential read/write working for this use case. But I can see 2 options to enable read and write not blocking each others.

    The first will be to copy the 2 ChannelInput/OutputStream wrapper classes and remove the blocking check. This will remove the lock contingency as the lock will not be requested. But this also would result on potential unexpected behavior if the blocking configuration is changed.

    The other would be to wrap the SocketChannel by a delegating ReadableByteChannel and WriteableByteChannel before calling the static factory method. This way the SelectableChannel specific part will be simply by passed. This will also remove the blocking check (as the actual channel can still be re configured) and can provide the same unexpected behavior in case the SocketChannel is reconfigured as non blocking.

  2. First of all, congrats Trisha for joining the Head First Bandwagon 🙂 as it has been the most favorite book for all the Java people out there!! Excited to get the copy of the 3rd ed when it is released. Any idea when and where it gets shipped to India?

Leave a Reply

Your email address will not be published.

This site uses Akismet to reduce spam. Learn how your comment data is processed.