8
\$\begingroup\$

I'm somewhat new to network programming. I have a server that uses Ubuntu, which needs to send data quickly to about 50 clients.

As of now, I have about 50 concurrent connections (of course), and it needs to be scalable up to 500.

I've created a JSON-based protocol to handle the streams and binary. Can these 2 classes be reviewed for any potential problems, bad techniques, etc?

JSONServer

package com.wordpress.waffalware.tcpson;

import java.io.IOException;
import java.net.InetAddress;
import java.net.InetSocketAddress;
import java.net.ServerSocket;
import java.net.Socket;
import java.net.SocketTimeoutException;
import java.util.ArrayList;

import org.json.JSONObject;

public class JSONServer {

    private int connectedSocketLimit = 10000;
    private RequestCallback onRequest;
    private ErrorCallback onError;

    private ServerSocket server;
    private boolean connected;

    public final ArrayList<JSONClient> clients = new ArrayList<JSONClient>();

    public void setOnRequest(RequestCallback handler) {
        onRequest = handler;
    }

    public void setOnError(ErrorCallback handler) {
        onError = handler;
    }

    public boolean isConnected() {
        return connected;
    }

    public void start(int port) throws IOException {
        server = new ServerSocket();
        server.setSoTimeout(5000);
        server.setReuseAddress(true);
        server.setPerformancePreferences(1, 0, 0);
        server.bind(new InetSocketAddress(InetAddress.getByAddress(new byte[] {
                0, 0, 0, 0 }), port));

        Thread accepter = new Thread(new Runnable() {
            @Override
            public void run() {
                while (connected) {
                    Socket client = null;
                    try {
                        client = server.accept();
                    } catch (SocketTimeoutException e) {
                        continue;
                    } catch (IOException e) {
                        if (onError != null) {
                            onError.onError(e);
                        }
                    }
                    if (client != null) {
                        handleClient(new JSONClient(client));
                    }
                }
            }
        }, "JSONServer-accepter");

        connected = true;
        accepter.start();
    }

    public void close() {
        connected = false;
        if (server != null) {
            try {
                server.close();
            } catch (IOException e) {
                e.printStackTrace();
            }
            server = null;
        }
    }

    @Override
    protected void finalize() throws Throwable {
        close();
        super.finalize();
    }

    private void handleClient(final JSONClient client) {
        Thread clientThread = new Thread(new Runnable() {
            @Override
            public void run() {
                try {
                    clients.remove(client);
                    clients.add(client);
                    client.setSoTimeout(5000);
                    while (connected) {
                        JSONPacket request = client.readPacket();
                        if (request == null) {
                            break;
                        }
                        JSONPacket response;
                        if (onRequest != null) {
                            response = onRequest.onRequest(request);
                        } else {
                            response = new JSONPacket(new JSONObject());
                        }

                        client.writePacket(response);

                        if (clients.size() > connectedSocketLimit) {
                            break;
                        }
                    }
                } catch (SocketTimeoutException e) {
                    if (onError != null) {
                        onError.onError(e);
                    }
                } catch (IOException e) {
                    e.printStackTrace();
                } finally {
                    client.close();
                    clients.remove(client);
                }
            }
        }, "JSONServer-clientHandler");
        clientThread.start();
    }

    public interface RequestCallback {
        public JSONPacket onRequest(JSONPacket request);
    }

    public interface ErrorCallback {
        public void onError(Exception e);
    }

}

JSONClient

package com.wordpress.waffalware.tcpson;

import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.net.InetSocketAddress;
import java.net.Socket;
import java.net.SocketAddress;
import java.net.SocketException;
import java.nio.ByteBuffer;

import org.json.JSONObject;

public class JSONClient {

    private boolean connected;
    private Socket client;

    private static final byte[] VERSION1 = new byte[] { 0x17, 0x78 };

    public static JSONPacket getResponse(String host, int port,
            JSONPacket request) throws IOException {
        JSONClient client = new JSONClient();
        try {
            client.setSoTimeout(5000);
            client.connect(new InetSocketAddress(host, port));

            client.writePacket(request);
            return client.readPacket();

        } finally {
            client.close();
        }
    }

    public JSONClient() {
        client = new Socket();
    }

    protected JSONClient(Socket baseClient) {
        client = baseClient;
        connected = baseClient.isConnected() && !baseClient.isClosed();
    }

    public boolean isConnected() {
        return connected;
    }

    public void connect(SocketAddress endpoint) throws IOException {
        client.connect(endpoint);
        connected = true;
    }

    public void close() {
        connected = false;
        try {
            client.close();
        } catch (IOException e) {
            e.printStackTrace();
        }
        client = new Socket();
    }

    /**
     * Writes <code>packet</code> to the <code>Socket</code>.
     * 
     * @param packet
     *            The <code>JSONPacket</code> to write to <code>Socket</code>.
     * @throws IOException
     *             If an IO/Error has occurred.
     */
    public void writePacket(JSONPacket packet) throws IOException {
        try {
            OutputStream str = client.getOutputStream();
            byte[] payloadRaw = packet.getPayload().toString().getBytes();
            byte[] extraData = packet.getExtraData();
            byte[] buffer = ByteBuffer
                    .allocate(14 + payloadRaw.length + extraData.length)
                    .put(VERSION1).put(intToByteArray(0))
                    .put(intToByteArray(payloadRaw.length))
                    .put(intToByteArray(extraData.length)).put(payloadRaw)
                    .put(extraData).array();
            str.write(buffer);
            str.flush();
        } catch (IOException e) {
            close();
            throw e;
        }
    }

    /**
     * Reads and creates a <code>JSONPacket</code> from the <code>Socket</code><br>
     * <br>
     * If <code>null</code> is returned, the other side of the
     * <code>Socket</code> should be treated as incompatible, and the
     * <code>Socket</code> be closed immediately.
     * 
     * @param str
     *            The stream to read the <code>JSONPacket</code> from.
     * @return A <code>JSONPacket</code> representing the data from the stream,
     *         or <code>null</code> if there were no bytes to read or the data
     *         does not follow the format.
     * @throws IOException
     *             If an IO/Error has occurred.
     */
    public JSONPacket readPacket() throws IOException {
        try {
            byte[] tempBytes;

            JSONPacket ret = null;

            InputStream str = client.getInputStream();

            tempBytes = new byte[14];
            if (str.read(tempBytes, 0, tempBytes.length) == tempBytes.length) {

                if (VERSION1[0] == tempBytes[0] & VERSION1[1] == tempBytes[1]) {

                    // int reservedValue = byteArrayToInt(tempBytes, 2);
                    int payloadSize = byteArrayToInt(tempBytes, 6);
                    int extraDataSize = byteArrayToInt(tempBytes, 10);

                    if (payloadSize >= 0 & extraDataSize >= 0) {
                        tempBytes = new byte[payloadSize];

                        if (str.read(tempBytes, 0, tempBytes.length) == tempBytes.length) {
                            JSONObject payloadData = new JSONObject(new String(
                                    tempBytes));

                            tempBytes = new byte[extraDataSize];

                            if (str.read(tempBytes, 0, tempBytes.length) == tempBytes.length) {
                                ret = new JSONPacket(payloadData, tempBytes);
                            }

                        }
                    }

                }
            }

            return ret;
        } catch (IOException e) {
            close();
            throw e;
        }
    }

    public void setSoTimeout(int timeout) throws SocketException {
        // TODO: Should the connection stop if an exception is thrown?
        client.setSoTimeout(timeout);
    }

    private static int byteArrayToInt(byte[] buffer, int offset) {
        return (buffer[offset] & 0xFF) << 24
                | (buffer[offset + 1] & 0xFF) << 16
                | (buffer[offset + 2] & 0xFF) << 8 | buffer[offset + 3] & 0xFF;
    }

    private static byte[] intToByteArray(int buffer) {
        return new byte[] { (byte) ((buffer >> 24) & 0xFF),
                (byte) ((buffer >> 16) & 0xFF), (byte) ((buffer >> 8) & 0xFF),
                (byte) (buffer & 0xFF) };
    }

}

The JSONPacket is just a holding object for a byte array, and a JSONObject.

A long time ago, the accept function kept throwing an IOException because there were "too many open files". I suspected it was that I forgot to call Socket.close() after I called accept.

Main Points

  1. Are there any techniques I should be using?
  2. Does the code above properly close sockets?
  3. I have researched about java.nio and wondered, should I use that package in my current situation? I know it's meant for non-blocking sockets, and millions of connections, but I only expect to have up to 500 concurrent connections.
  4. Am I using threads correctly? 1 socket --> 1 connection
\$\endgroup\$
  • \$\begingroup\$ Off-topic, but what's wrong with big headers? \$\endgroup\$ – Kayla Mar 10 '14 at 21:32
7
\$\begingroup\$

There is a lot of ground to cover here...

General

ToString

You do not have a toString() on your JSONClient code. These are invaluable for debugging, etc.

Configuration

There are a lot of values which are hard-coded in your program. Things like IP addresses to bind to (you have the 'wild-card' 0.0.0.0 hardcoded) can be specified on a per-interface basis, rather than hard-coded.

Other items like timeouts and so on should be configurable as well.

Threads

You are using new Thread(...) a lot in your code.... but you are not setting the daemon-state of the threads with setDaemon(true). I would consider all your threads to be daemon.

I am undecided whether to recommend that you actually use an ExecutorService to organize your threads, or whether to customize your thread-name a different way. Basically, you are going to have 500 threads all called 'JSONServer-clientHandler'. This is not useful. It would be better to call them 'Thread 1', 'Thread 2', etc. But, that is silly too. I would highly recommend naming the thread after the client-side of the connection, something like:

"Client Handler for " + socket.getRemoteAddress()

Actually, for that benefit alone, I would recommend you continue using your Thread implementation, but you need to extract out the anaonymous Runnable class in to a more concrete class... even perhaps making the JSONClient a Runnable? The anonymous class for such an important piece of code is a problem.

Continuing on the thread theme, I don't like the anonymous class in the Server's start method either.... The server should be a complete class with a run() method too

Socket Timeout

Why are you setting soTimeout(5000) on the ServerSocket?

Waiting 5 seconds is not a problem for a ServerSocket.... and, then it throws a timout exception, and 'continues' to block again. Why not just do the sensible thing and not have the soTimeout() set at all (or set it to 0)?

Socket Closing

You specifically ask if the sockets are being closed properly.

The method you use will close all client sockets, yes, but the mechanism is cumbersome.

You should be using the Java7 try-with-resources mechanisms for doing this. It will make both the exception and normal-case usage a lot better.

Concurrency

You have the connected boolean value, but this is not used in a thread-safe way... the variable is not atomic, it is not volatile, and it is not read in a synchronized way, thus, it is possible that the server thread will never read it as it changes value.

additionally, this code here:

clients.remove(client);
clients.add(client);

is called from a new thread, but it affects the client List which is shared on all threads. But that instance is neither synchronized nor concurrent. There will be corruption to that list at some point.

Out-of-place logic

if (clients.size() > connectedSocketLimit) {
    break;
}

The above code is in the client-handling loop.

This logic is mis-placed. You should not allow a client thread to start if it is going to violate a limit..... instead, you are allowing clients to start, and only after they have done a request-response do you (not-thread-safe) check the limits. Then you terminate the thread that checked the situation, instead of the thread which caused the situation.... Checking an error condition after the error has happened is not a best practice. Checking a condition before entering a broken state is best-practice

While we are on this topic... if you are only expecting up to 500 clients, why is the limit set at 10,000? This seems inappropriate

JSON Client

This is added to the public final ArrayList<JSONClient> clients List, but, before it is added, it is removed:

clients.remove(client);
clients.add(client);

but your JSONClient code does not implement equals()/hashCode() so the remove will do an identity check to do the comparison, and thus the remove() will never remove anything because you can never add a client twice. Why have the remove() ?

Additionally, it appears that your JSONClient class is used as both the client-side class, as well as the server-side handler. I don't like this model. You should separate those concerns, and, if needed provide some abstract-class functionality that both sides can use.

Having both concerns in a single class makes it complicated.


Performance

General Performance

  • Don't start tuning things until you know you need to. Why do you have this line:

    server.setPerformancePreferences(1, 0, 0);
    

    Do you know that connection-time is a problem? Are you just imagining that it may be?

Threads

In current Java versions there is no real limit to the number of threads you can have running. You are planning on about 500, and this is not particularly concerning. Obviously they will not all be able to be on-cpu at the same time, but that is OK. There is no need to change your thread model for this reason.

Using the NIO non-blocking mechanisms may help you reduce thread-count, but for the past 5 or so years there has been a lot of debate as to whether there is any performance benefit from that. It is my opinion that the complexity of using non-blocking channels with selectors, etc. is not worth the effort. Allocating a thread-per-socket is just fine.


Conclusion

Your code has enough critical problems that I think you need to consider a re-factor. The anonymous-class thread models make spotting the logic hard.

Additionally, there is a lot of stuff I did not cover here... the above details are just the most significant things I saw.

\$\endgroup\$
  • 1
    \$\begingroup\$ Good answer proving that concurrency and, in some way, networking is quite hard stuff to begin with and requires much, much patience : ) \$\endgroup\$ – guitar_freak Mar 13 '14 at 8:17
  • \$\begingroup\$ Using the NIO non-blocking mechanisms may help you reduce thread-count, but for the past 5 or so years there has been a lot of debate as to whether there is any performance benefit from that. It would be better if you explained it further with a reference. :) \$\endgroup\$ – Eray Erdin Feb 3 '18 at 14:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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