Socket server with multiple clients, sending messages to many clients without hurting liveliness

Question

I have a small socket server, and I need to distribute various messages from client-to-client depending on different conditionals.

However I think I have a small problem with livelyness in my current code, and is there anything wrong in my approach:

public class CuClient extends Thread
{

Socket socket = null;
ObjectOutputStream out;
ObjectInputStream in;
CuGroup group;

public CuClient(Socket s, CuGroup g)
{
    this.socket = s;
    this.group = g;
    out = new ObjectOutputStream(this.socket.getOutputStream());
    out.flush();
    in = new ObjectInputStream(this.socket.getInputStream());
}

@Override
public void run()
{
    String cmd = "";
    try {
        while (!cmd.equals("client shutdown")) {
            cmd = (String) in.readObject();
            this.group.broadcastToGroup(this, cmd);
        }
        out.close();
        in.close();
        socket.close();
    } catch (Exception e) {
        System.out.println(this.getName());
        e.printStackTrace();
    }
}

public void sendToClient(String msg)
{
    try {
        this.out.writeObject(msg);
        this.out.flush();
    } catch (IOException ex) {
    }
}

And my CuGroup:

public class CuGroup
{
private Vector<CuClient> clients = new Vector<CuClient>();


public void addClient(CuClient c)
{
    this.clients.add(c);
}

void broadcastToGroup(CuClient clientName, String cmd)
{
    Iterator it = this.clients.iterator();
    while (it.hasNext()) {
        CuClient cu = (CuClient)it.next();
        cu.sendToClient(cmd);
    }
}
}

And my main-class:

public class SmallServer
{
public static final Vector<CuClient> clients = new Vector<CuClient>(10);
public static boolean serverRunning = true;
private ServerSocket serverSocket;
private CuGroup group = new CuGroup();

public void body()
{
    try
    {
        this.serverSocket = new ServerSocket(1337, 20);
        System.out.println("Waiting for clients\n");
        do
        {
            Socket s = this.serverSocket.accept();
            CuClient t = new CuClient(s,group);
            System.out.println("SERVER: " + s.getInetAddress() + " is connected!\n");
            t.start();
        } while (this.serverRunning);
    } catch (IOException ex)
    {
        ex.printStackTrace();
    }
}

public static void main(String[] args)
{
    System.out.println("Server");
    SmallServer server = new SmallServer();
    server.body();
}
}

Consider the example with many more groups, maybe a Collection of groups. If they all synchronize on a single Object, I don't think my server will be very fast.

I there a pattern or something that can help my liveliness?

Answer 1

The biggest flaw in terms of correctness is that ObjectOutputStream is not a thread-safe class. By calling sendToClient from a variety of threads, you have exposed yourself to a large race condition where two outgoing messages get mixed with each other.

Two simple but inefficient solutions: 1) synchronize sendToClient, 2) create a sender thread for each client which reads from a BlockingQueue, and change sendToClient to add to that queue. The former is wasteful because a single high-latency client will bottleneck others. The latter is wasteful because it requires double the number of threads. As @spender said, asynch IO would indeed be better for this code, but it's certainly less straighforward to code.

Answer 2

It looks like you need to do a bit of study into asynchronous IO (i.e. IO that does not block). Using a Thread per client will never scale that well.

There's a few answers to this question that might point you in the right direction:

Asynchronous IO in Java?