Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I tried to make a Server using Java with a Thread Pool. Any review is welcome:

/* User: [email protected] Date: 21/02/15 Time: 21:12 */

import java.io.IOException;
import java.net.ServerSocket;
import java.net.Socket;
import java.util.Stack;

public class MyConnectionAccepter {

    private Stack<MySocketThread> mySocketThreads = new Stack<MySocketThread>();
    private volatile int currentNumberOfConnections = 0;

    public MyConnectionAccepter() {

        MySocketThread mySocketThreadOne = new MySocketThread(this);
        MySocketThread mySocketThreadTwo = new MySocketThread(this);

        mySocketThreadOne.setDaemon(true);
        mySocketThreadTwo.setDaemon(true);

        mySocketThreadOne.start();
        mySocketThreadTwo.start();

        mySocketThreads.push(mySocketThreadOne);
        mySocketThreads.push(mySocketThreadTwo);

    }

    public void start() throws IOException {

        ServerSocket serverSocket = new ServerSocket(8888);

        while (true) {
            while (currentNumberOfConnections < 2) {
                Socket accept = serverSocket.accept();
                MySocketThread mySocketThread = mySocketThreads.pop();
                mySocketThread.setSocket(accept);
                currentNumberOfConnections++;
            }
        }

    }

    public void informIAmDone(MySocketThread mySocketThread) {
        mySocketThreads.push(mySocketThread);
        currentNumberOfConnections--;
    }
}

and

/* User: [email protected] Date: 21/02/15 Time: 21:04 */

import java.io.IOException;
import java.net.Socket;
import java.util.Scanner;

public class MySocketThread extends Thread {

    private volatile Socket socket;
    MyConnectionAccepter myConnectionAccepter;

    public MySocketThread(MyConnectionAccepter myConnectionAccepter) {
        this.myConnectionAccepter = myConnectionAccepter;
    }

    @Override
    public synchronized void run() {
        serve();
    }

    public void setSocket(Socket socket) {
        this.socket = socket;
    }

    public void serve() {
        while(socket == null) {

        }
        while (socket != null) {
            Scanner scanner = null;
            try {
                scanner = new Scanner(socket.getInputStream());
            } catch (IOException e) {
                e.printStackTrace();
            }
            String readLine;
            while (!(readLine = scanner.nextLine()).equals("bye")) {
                System.out.println(readLine);
            }
            try {
                socket.close();
            } catch (IOException e) {
                e.printStackTrace();
            }
            socket = null;
            myConnectionAccepter.informIAmDone(this);
        }
        serve();
    }

}

and the Test Class:

/* User: [email protected] Date: 21/02/15 Time: 21:18 */

import java.io.IOException;

public class MyTestClass {

    public static void main(String[] args) throws IOException {
        MyConnectionAccepter myConnectionAccepter = new MyConnectionAccepter();
        myConnectionAccepter.start();
    }
}

My goal was to allow 2 connection at any time. So I can connect from Terminal like this:

Korays-MacBook-Pro:~ koraytugay$ telnet localhost 8888
Trying ::1...
Connected to localhost.
Escape character is '^]'.

But a new Thread will not be created, an available Thread will popped from the Stack and that will be used. If no Threads are available in the pool, the new connection will simply need to wait until some other connection closes. Closing a connection does not stop a Thread, it only makes it free.

Any review is welcome from any aspect.

share|improve this question

1 Answer 1

Generic code review

This is know as a busy loop:

    while(socket == null) {

    }

Not a good idea in any language.
Do you hear your fans spin up when your threads start executing this loop?

You should probably block the thread until the condition becomes true: See is there a 'block until condition becomes true' function in java?

Recursively calling your own function is not a good idea:

 public void serve() {
    .. DO WORK ITEM 1
    serve();
}

That would not explode immediately. But in a production system that is running a long time that will eventually fill up the stack and cause the application to crash. People will spend years trying to work out why it crashed (because its not obvious that this is a slow and steady memory leak that is undetectable).

Why not put that in a loop?

public synchronized void run() {
    while(!finished) {
        serve();
    }
     // remove the recursive call inside serve()
}

This is another hidden busy loop:

        while (currentNumberOfConnections < 2) {
            Socket accept = serverSocket.accept();
            MySocketThread mySocketThread = mySocketThreads.pop();
            mySocketThread.setSocket(accept);
            currentNumberOfConnections++;
        }

When you have two active connections then this will spin like mad and just eat CPU resources. Put another block in there so that the thread is paused until your worker threads can keep going

Design Review:

I think your socket and your threads are to tightly coupled. I would rather separate them and make them independent of each other.

Have your socket class accepts incoming requests and for each request create a work item in a queue. That way it keeps accepting requests and does not need to drop any. If the threads get there two slowly then the client will close the connection and your thread will see this when it tries to service the client rquest.

One the other side make your threads collect work items from the queue (mentioned above). You then can add more threads without the socket class knowing about any of the threads (they just know about the queue).

share|improve this answer

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.