Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I am learning Java's NIO and as an exercise, I have implemented a simple chat server. I would like to ask if I am using NIO's features in the correct way.

public class ChatServer implements Runnable {
    private static final int BUFFER_SIZE = 256;
    private static final int DEFAULT_PORT = 10523;
    private static final String WELCOME_MESSAGE = "Welcome to NioChat!\n";

    private final int mPort;
    private ServerSocketChannel mServerChannel;
    private Selector mSelector;
    private ByteBuffer mBuffer = ByteBuffer.allocate(BUFFER_SIZE);
    private final ByteBuffer mWelcomeBuffer = ByteBuffer.wrap(WELCOME_MESSAGE
            .getBytes());

    public ChatServer(int port) throws IOException {
        mPort = port;
        mServerChannel = ServerSocketChannel.open();
        mServerChannel.socket().bind(new InetSocketAddress(port));
        mServerChannel.configureBlocking(false);
        mSelector = Selector.open();
        mServerChannel.register(mSelector, SelectionKey.OP_ACCEPT);
    }

    public static void main(String[] args) throws IOException {
        ChatServer server = new ChatServer(DEFAULT_PORT);
        new Thread(server).start();
    }

    @Override
    public void run() {
        try {
            System.out.println("Server started on port " + mPort);

            Iterator<SelectionKey> iter;
            SelectionKey key;
            while (mServerChannel.isOpen()) {
                mSelector.select();
                iter = mSelector.selectedKeys().iterator();
                while (iter.hasNext()) {
                    key = iter.next();
                    iter.remove();

                    if (key.isAcceptable())
                        handleAccept(key);
                    if (key.isReadable())
                        handleRead(key);
                }
            }
        } catch (IOException e) {
            System.out.println("IOException, server on port " + mPort
                    + " terminating. Stack trace:");
            e.printStackTrace();
        }
    }

    private void handleAccept(SelectionKey key) throws IOException {
        SocketChannel sc = ((ServerSocketChannel) key.channel()).accept();
        String address = (new StringBuilder(sc.socket().getInetAddress()
                .toString())).append(":").append(sc.socket().getPort())
                .toString();
        sc.configureBlocking(false);
        sc.register(mSelector, SelectionKey.OP_READ, address);
        sc.write(mWelcomeBuffer);
        mWelcomeBuffer.rewind();
        System.out.println("Accepted connection from: " + address);
    }

    private void handleRead(SelectionKey key) throws IOException {
        SocketChannel ch = (SocketChannel) key.channel();
        StringBuilder sb = new StringBuilder();

        mBuffer.clear();
        int read = 0;
        while ((read = ch.read(mBuffer)) > 0) {
            mBuffer.flip();
            byte[] bytes = new byte[mBuffer.limit()];
            mBuffer.get(bytes);
            sb.append(new String(bytes));
            mBuffer.clear();
        }
        String msg;
        if (read < 0) {
            msg = key.attachment() + " left the chat.\n";
            ch.close();
        } else {
            msg = key.attachment() + ": " + sb.toString();
        }

        System.out.println(msg);
        broadcast(msg);
    }

    private void broadcast(String msg) throws IOException {
        ByteBuffer messageBuffer = ByteBuffer.wrap(msg.getBytes());
        for (SelectionKey key : mSelector.keys()) {
            if (key.isValid() && key.channel() instanceof SocketChannel) {
                SocketChannel channel = (SocketChannel) key.channel();
                channel.write(messageBuffer);
                messageBuffer.rewind();
            }
        }
    }
}
share|improve this question
up vote 2 down vote accepted

Bad support for fragmented tcp packets

StringBuilder sb = new StringBuilder();

mBuffer.clear();
int read = 0;
while ((read = ch.read(mBuffer)) > 0) {
    mBuffer.flip();
    byte[] bytes = new byte[mBuffer.limit()];
    mBuffer.get(bytes);
    sb.append(new String(bytes));
    mBuffer.clear();
}

The above implementation only works reliable with messages smaller than 567 (IPV4) or 1200 (ipv6) bytes, because larger messages can come in fragmented in multiple chunks with large intervals in case of packet loss. This will result is half send chat messages that are broadcasted

Using default charset

You are not specifying any charset when reading the messages, this will result in platform and even java dependent behaviour when reading special characters.

Manually using stringbuilder

String address = (new StringBuilder(sc.socket().getInetAddress()
                .toString())).append(":").append(sc.socket().getPort())
                .toString();

You don't need to use stringbuilder if you already have all the used arguments on 1 line, the compiler will automatically add a stringbuilder for you.

String address = sc.socket().getInetAddress() + ":" + sc.socket().getPort();
share|improve this answer
    
Thanks, how should I correctly handle fragmented TCP packets? – ΔλЛ Apr 13 '16 at 12:26
1  
This will require redesign, you should keep a bytebuf per connection, and store all the read bytes into that. If you find a newline character in the incoming data, you should send the whole bytebuf as a message, and empty it, so you can start over again – Ferrybig Apr 13 '16 at 13:25

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.