Java Multithreaded client/server - java.net.SocketException: Socket closed

Question

I have to write a multithreaded client and server using Java's socket api. Both the client and server are multithreaded, so the server can handle multiple connections and the client can test the server's ability to handle connections.

My code is here: https://github.com/sandyw/Simple-Java-Client-Server

I am having a couple, probably related issues. One, occasionally one of the client threads will throw

java.net.SocketException: Socket closed
    at java.net.SocketInputStream.socketRead0(Native Method)
    at java.net.SocketInputStream.read(SocketInputStream.java:129)
    at sun.nio.cs.StreamDecoder.readBytes(StreamDecoder.java:264)
    at sun.nio.cs.StreamDecoder.implRead(StreamDecoder.java:306)
    at sun.nio.cs.StreamDecoder.read(StreamDecoder.java:158)
    at java.io.InputStreamReader.read(InputStreamReader.java:167)
    at java.io.BufferedReader.fill(BufferedReader.java:136)
    at java.io.BufferedReader.readLine(BufferedReader.java:299)
    at java.io.BufferedReader.readLine(BufferedReader.java:362)
    at ClientThread.run(ClientThread.java:45)
    at java.lang.Thread.run(Thread.java:680)

Which from its position means the server is closing its socket before the client is done reading from it. How can I prevent this?

Also, even counting the threads which throw the Socket closed exception, it seems that not all the server threads are sending their output. For example, 23 of the 30 server threads will send their output, but only 22 of the client threads will receive anything. The communications are obviously getting lost somewhere, how do I prevent this?

Solution

Your problem is this in ClientThread:

private static Socket socket = null;

That means all threads are sharing the same socket instance for all instances of ClientThread. That means your sockets are going to get out of sync with the state of the conversation. Your conversation is:

Client states:

1. Client connects
2. Client sends request
3. Client waits for response
4. Client receives response
5. Client close socket (should be done by each).

Server states:

1. Server waits for client
2. Server reads request
3. Server processes command
4. Server sends response
5. Server closes sockets

However, you have 30 conversations all trying to be in different states at the same time, and the server and client can't keep track of them. First thread creates the socket sends a request moves to state 2, while it's waiting another threads creates another socket, writes it to moves to state 2, when the First thread wakes up it then starts talking over the new socket that was created by thread 2 which hasn't finished processing it's command. Now a third starts and overwrites that reference again, so on and so on. The first command won't ever be read because it lost the reference to the original socket when thread 2 overwrote it, and it starts reading thread 2's command and spitting it out. Once thread 1 makes it to the close statements it closes the socket down while other threads are trying to read/write to it, and bang the exception is tossed.

Essentially each ClientThread should create their own socket instance, and then each conversation can occur independently than the other conversations going on. Think if you wrote your client to be single threaded. Then started up two separate processes (run your Java app twice). Each process would create their own socket and each conversation would work independently. What you have now is one socket with 30 threads shouting commands over one megaphone to the server. Work can't proceed in an orderly fashion when everyone is shouting over the same megaphone.

So in summary change remove static modifier from socket member in ClientThread, and it should start working pretty well.

Just as an aside never release this code into the world. It has serious security problems because clients can execute any command at the security level your server process is running. So very easily anyone could own your machine or capture your account with relatively ease. Executing commands like that from the client means they could send:

sudo cat /etc/passwd

And capture your password hashes for example. I think you are just learning, but I felt like you should be alerted to what you're doing to be on the safe side.

Another thing is the server is only closing down sockets if the conversation went as expected. You really should move the close() calls into the finally block on the try block you have. Otherwise if a client closes its socket prematurely (it happens) then your server will leak sockets and eventually it will run out of sockets in the OS.

public void run() {
   try {
   } catch( SomeException ex ) {
      logger.error( "Something bad happened", ex );
   } finally {
      out.close();   <<<< not a bad idea to try {} finally {} these statements too.
      in.close();    <<<< not a bad idea to try {} finally {} these statements too.
      socket.close();
   }
}

Another thing you might want to explore is using Thread pools on the server instead of newing up a thread for every new connection you get. In your simple example it's easy to play around with this, and it works. However, if you were building a real server a thread pool has two major contributions. 1. creating threads has an overhead associated with it so you can get some performance response time by have a thread sitting around waiting to service incoming requests. You can save yourself sometime to respond to clients. 2. More importantly a physical computer can't endlessly create threads. If you had lots of clients say 1000+ your machine would be hard pressed to answer all of those using 1000 threads. A thread pool means you create max number of threads, say 50 threads, and each thread will be used multiple times to process each request. As new connections come in they wait for a thread to free up before being processed. If you get too many connections coming in the clients will timeout as opposed to destroying your machine requiring a reboot. It allows you to get through more requests faster while keeping you from dying should too many connections come in at once.

Finally, a close socket exception can happen for many valid reasons. Often if a client just shuts down in the middle of a conversation the server will get that exception. It's best to properly shutdown and clean up yourself when that happens. You can't ever prevent it is my point. You just have to respond to it.

Comments

[Tick-Tock]

Thank you! That makes perfect sense, I didn't put the static modifier there so I was blind to it. I've been banging my head against this for days.