tags:

views:

1384

answers:

3

This is a follow up to:

this question

Basically, I have a server loop that manages a connection to one solitary client. At one point in the loop, if a ClientSocket exists it attempts a read to check if the client is still connected:

if (bufferedReader.read()==-1 ) {
       logger.info("CONNECTION TERMINATED!");
       clientSocket.close(); 
       setUpSocket(); //sets up the server to reconnect to the client
}else{
        sendHeartBeat(); //Send a heartbeat to the client
}

The problem is, that once a socket has been created the application will hang on the read, I assume waiting for data that will never come, since the client never sends to the server. Before this was OK, because this correctly handled disconnects (the read would eventually fail when the client disconnected) and the loop would attempt reestablish the connection. However, I now have added the above sendHeartBeat() method, which periodically lets the client know the server is still up. If the read is holding the thread then the heartbeats never happen!

So, I assume I am testing if the connection is still up incorrectly. I could, as a quick hack, run the bufferedReader.read() in a seperate thread, but then I'll have all sorts of concurrency issues that I really don't want to deal with.

So the question is a few fold: 1) Am I checking for a client disconnect correctly? 2) If not, how should I do it? 3) If I am doing it correctly how I do I get the read to not hold the process hostage? Or is threading the only way?

A: 

You should add error checking in your disconnection detection. Sometimes an IOException may be thrown when the connection to the other end is lost.

I am afraid that threading is unavoidable here. If you don't want to block the execution of your code, you need to create a separate thread.

kgiannakakis
So you are saying I have to run the bufferedReader.run() in a seperate thread, correct?
windfinder
Sooner or later you will need threading. It would be better to out bufferedReader.read in a separate thread. In fact all socket communications should be in a thread of their own.
kgiannakakis
+2  A: 

When you create your socket, first set a timeout:

private int timeout    = 10000;
private int maxTimeout = 25000;

clientSocket.setSoTimeout(timeout);

With this, if a read times out you'll get java.net.SocketTimeoutException (which you have to catch). Thus, you could do something like this, assuming you've previously set the SO_TIMEOUT as shown above, and assuming that the heartbeat will always get a response from the remote system:

volatile long lastReadTime;

try {
    bufferedReader.read();
    lastReadTime = System.currentTimeMillis();
} catch (SocketTimeoutException e) {
    if (!isConnectionAlive()) {
        logger.info("CONNECTION TERMINATED!");
        clientSocket.close(); 
        setUpSocket(); //sets up the server to reconnect to the client
    } else {
        sendHeartBeat(); //Send a heartbeat to the client
    }
}

public boolean isConnectionAlive() {
    return System.currentTimeMillis() - lastReadTime < maxTimeout;
}

A common way of handling this is setting the timeout to some number (say 10 seconds) and then keeping track of the last time you successfully read from the socket. If 2.5 times your timeout have elapsed, then give up on the client and close the socket (thus sending a FIN packet to the other side, just in case).

If the heartbeat will not get any response from the remote system, but is just a way of ultimately generating an IOException earlier when the connection has fallen down, then you could do this (assuming that the sendHeartBeat itself will not throw an IOException):

try {
    if (bufferedReader.read() == -1) {
        logger.info("CONNECTION TERMINATED with EOF!");
        resetConnection();
    }
} catch (SocketTimeoutException e) {
    // This just means our read timed out ... the socket is still good
    sendHeartBeat(); //Send a heartbeat to the client
} catch (IOException e) {
    logger.info("CONNECTION TERMINATED with Exception " + e.getMessage());
    resetConnection();
}

....

private void resetConnection() {
    clientSocket.close(); 
    setUpSocket(); //sets up the server to reconnect to the client
}
Eddie
But the read will not return a -1 if the client is there, correct?
windfinder
I added a code example to show
Eddie
I improved the code example to use more of your code.
Eddie
Actually, the connection could still exist in your code if nothing has been sent for time > maxTimeout, but the client is still connected.
windfinder
But in that case, you would have sent two heartbeats with no response, so that implies that the remote side is no longer talking.
Eddie
The client does not heartbeat back at all, and even if it did, there could be a long period of "lag" for many reasons.
windfinder
I'll update my answer to handle this case.
Eddie
+1  A: 

You are checking correctly, you can should add a try catch with IOException in case it occurs.

There is a way to avoid threading, you can use a Selector with a non-bloking socket.

public void initialize(){
  //create selector
  Selector selector = Selector.open();
  ServerSocketChannel acceptSocket = ServerSocketChannel.open();
  acceptSocket.configureBlocking(false);
  String bindIp = "127.0.0.1";
  int bindPort = 80;
  acceptSocket.socket().bind(new InetSocketAddress(bindIp, bindPort));
  //register socket in selector for ACCEPT operation
  acceptSocket.register(selector, SelectionKey.OP_ACCEPT);
  this.selector = selector;
  this.serverSocketChannel = serverSocketChannel;
}

public void serverStuff() {
   selector.select(maxMillisecondsToWait);
   Set<SelectionKey> selectedKeys = selector.selectedKeys();
   if( selectedKeys.size() > 0 )
   {
      if( key.isAcceptable() ){
        //you can accept a new connection
        SocketChannel clientSk = serverSocketChannel.accept();
        clientSk.configureBlocking(false);
        //register your SocketChannel in the selector for READ operations
    clientSk.register(selector, SelectionKey.OP_READ);
      } else if( key.isReadable() ){
        //you can read from your socket.
        //it will return you -1 if the connection has been closed
      }
   }

   if( shouldSendHeartBeat() ){
     SendHeartBeat
   }
}
Franco