Multi-threaded server socket

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 clients = new ArrayList();

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

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

StackExchange Code Review Q#43995, answer score: 6