This discussion is archived
9 Replies Latest reply: Jul 20, 2012 5:26 AM by gimbal2 RSS

NIO server : closing client connections properly

tmazight Newbie
Currently Being Moderated
Hi,


I have a NIO server which have some time to do 2 kinds of disconnections :

Send a message to the client and then disconnect him by calling channel.close() and key.cancel().
don't send a message and directly disconnect the client by calling channel.close() and key.cancel().

my Select :

     
public void startServer() throws IOException {
          while (true) {
               try {
                    // Process any pending changes
                    synchronized (this.changeRequests) {
                         Iterator<ChangeRequest> changes = this.changeRequests
                                   .iterator();
                         while (changes.hasNext()) {
                              ChangeRequest change = (ChangeRequest) changes.next();
                              switch (change.type) {
                              case ChangeRequest.CHANGEOPS:
                                   SelectionKey key = change.socket.keyFor(selector);

                                   if (key != null) {
                                        if (key.isValid())
                                             key.interestOps(change.ops);
                                   }
                              }
                         }
                         this.changeRequests.clear();
                    }
                    numberOfKeys = 0;
                    numberOfKeys = selector.select();
                    if (numberOfKeys == 0) {
                         // Fermeture des connexion si la fermeture a été demandé et
                         // qu'il y a aucun événement
                         
                         continue; // None of request available
                    }
                    // Get iterator through the selected keys list
                    Iterator<SelectionKey> iterKeys = selector.selectedKeys()
                              .iterator();
                    while (iterKeys.hasNext()) {
                         try {
                              SelectionKey selectedKey = (SelectionKey) iterKeys
                                        .next();
                              iterKeys.remove();
                              // Verify the key validity
                              if (!selectedKey.isValid()) {
                                   logger.error("Received key is invalid");
                                   continue;
                              } else {
                                   if (selectedKey.isValid()
                                             && selectedKey.isAcceptable()) {
                                        // Accept the client request
                                        accept(selectedKey);
                                   }
                                   if (selectedKey.isValid()
                                             && selectedKey.isReadable()) {
                                        readMessage(selectedKey);
                                   }
                                   if (selectedKey.isValid()
                                             && selectedKey.isWritable()) {
                                        write(selectedKey);
                                   }
                              }

                         } catch (Exception e) {
                              logger.error("Exception au cours de l'iteration sur le resultat du select() ");
                              logger.error(e.getMessage());
                              e.printStackTrace();
                         }
                         finally {
                              synchronized (this.closeRequests) {
                                   Iterator<Voie> closes = this.closeRequests.iterator();
                                   while (closes.hasNext()) {
                                        Voie voie = (Voie) closes.next();
                                        if (voie.getChannelPrio() != null
                                                  && voie.getKeyPrio() != null) {
                                             voie.getChannelPrio().close();
                                             voie.getKeyPrio().cancel();
                                        }
                                        if (voie.getChannelNPrio() != null
                                                  && voie.getKeyNPrio() != null) {
                                             voie.getChannelNPrio().close();
                                             voie.getKeyNPrio().cancel();
                                        }
                                   }
                              }
                         }
                    }

               } catch (Exception e) {
                    logger.error("Exception au cours du changement des option d'interêt pour les clés");
                    logger.error(e.getMessage());
                    e.printStackTrace();
               }
Send method called by worker threads:

     
public void send(SocketChannel socket, byte[] data) {
          synchronized (this.changeRequests) {
               // Indicate we want the interest ops set changed
               this.changeRequests.add(new ChangeRequest(socket,
                         ChangeRequest.CHANGEOPS, SelectionKey.OP_WRITE));
          }
          // And queue the data we want written
          synchronized (this.pendingData) {
               Queue<ByteBuffer> queue = (LinkedList<ByteBuffer>) this.pendingData
                         .get(socket);

               queue.add(ByteBuffer.wrap(data));
          }

          selector.wakeup();
     }
write method: called by the event on selector

     
private void write(SelectionKey key) throws IOException {
          SocketChannel socketChannel = (SocketChannel) key.channel();

          synchronized (this.pendingData) {
               Queue<ByteBuffer> queue = (LinkedList<ByteBuffer>) this.pendingData
                         .get(socketChannel);
               nbrWrites++;
               // Write until there's not more data ...
               while (!queue.isEmpty()) {
                    ByteBuffer buf = (ByteBuffer) queue.remove();
                    socketChannel.write(buf);
               }
               key.interestOps(SelectionKey.OP_READ);
               selector.wakeup();
          }

     }
and finally the close method

     
public void closeConnection(Voie voie) {
          synchronized (this.closeRequests) {
               // Indicate we want the interest ops set changed
               this.closeRequests.add(voie);
          }
     }
i'm really blocked here, i guess and doing the closing of the connection wrong way, some times i getCanceledKeyException, closedChannelException when the selector invokes write() and sometimes i get nullPointerException when selector invokes read() and generally the client can't connect second time( something like key is not deleted...) the second connection don't make an event on selector. and all of this happens only after calling closeConnection()

I saw on internet that is better to make operations on key in the main thread (which is handling the selector) thats why i created a closeRequest queue.

Can you plz help me redesign this.

best regards

Edited by: tmazight on Jul 19, 2012 12:35 PM

Edited by: tmazight on Jul 19, 2012 12:39 PM
  • 1. Re: NIO server : closing client connections properly
    gimbal2 Guru
    Currently Being Moderated
    Before our local NIO experts come along to try and answer this, do the right thing and repost all code using \
     tags to make it readable. After 80 posts you should know that.                                                                                                                                                                                                                                                                                                                                                                        
  • 2. Re: NIO server : closing client connections properly
    tmazight Newbie
    Currently Being Moderated
    Done, thanks gimbal
  • 3. Re: NIO server : closing client connections properly
    EJP Guru
    Currently Being Moderated
    Send a message to the client and then disconnect him by calling channel.close() and key.cancel().
    don't send a message and directly disconnect the client by calling channel.close() and key.cancel().
    Hold it right there. Channel.close() disconnects the client and cancels the key. SelectionKey.cancel() doesn't disconnect the client and does nothing if it's already cancelled. Therefore you can completely get rid of key.cancel() out of this sequence.
                                       continue;
                                  } else {
    You don't need 'else' after 'continue'.
                             } catch (Exception e) {
    Not adequate. You must catch IOException here first. If you get any IOException operating on any channel in non-blocking mode, the channel is broken and must be closed. No ifs or buts.
                                  synchronized (this.closeRequests) {
    You don't need this stuff. Just close the channel from any thread the moment you have finished with it.
                   } catch (Exception e) {
                        logger.error("Exception au cours du changement des option d'interêt pour les clés");
    This message isn't correct. It could theoretically be 'au cours du' close.
                   while (!queue.isEmpty()) {
                        ByteBuffer buf = (ByteBuffer) queue.remove();
                        socketChannel.write(buf);
                   }
    Not adequate. I can absoluetly and 100% guarantee that you are losing data here. You must pay attention to the return code of write(), and to whether the buffer still had data in it. If the return code is zero or the buffer still has data you must (a) not remove it from the queue, (b) compact and flip it again, (c) stop processing the queue, and (d) leave the interestOps = OP_WRITE.
         
    public void closeConnection(Voie voie) {
              synchronized (this.closeRequests) {
                   // Indicate we want the interest ops set changed
                   this.closeRequests.add(voie);
              }
         }
    See above. You don't need this. Just close the channel.
    >
    some times i getCanceledKeyException, closedChannelException when the selector invokes write()
    Make the fixes I am recommending and retest.
    and sometimes i get nullPointerException when selector invokes read()
    You haven't posted the reading code so I cannot possibly comment, but in any case any competent Java programmer should be able to fix a NullPointerException without outside assistance.
    and generally the client can't connect second time( something like key is not deleted...)
    You haven't posted the accept code so I cannot possibly comment.
    the second connection don't make an event on selector.
    You must have broken the OP_ACCEPT registration in the accept code.
    and all of this happens only after calling closeConnection()
    Which you don't need.
    I saw on internet that is better to make operations on key in the main thread (which is handling the selector) thats why i created a closeRequest queue.
    There's a lot of nonsense on the Internet, especially about NIO. The drivel about closing the channel and then cancelling the key is a good example. It is simply unnecessary, as a moment with the Javadoc shows. You don't need a close queue. I've encountered the belief before that you need a change-interestOps queue, but I'm afraid I don't believe it. And I certainly don't believe in separate read and write threads when using non-blocking NIO. There is no way you can get a separate read thread to work correctly and in a timely fashion, and a separate write thread is simply not necessary at all. The whole idea of NIO is to economize on threads.
    Can you plz help me redesign this.
    Easy. Get rid of all the queues, all the key cancels after closes, and all the threads. And don't believe everything you read on the Internet.
  • 4. Re: NIO server : closing client connections properly
    tmazight Newbie
    Currently Being Moderated
    Thanks EJP,

    i'm making fixes you recommended, here are accept and write methods:
         public void accept(SelectionKey selectedKey) {
              // TODO Auto-generated method stub
              try {
                   ServerSocketChannel server = (ServerSocketChannel) selectedKey
                             .channel();
                   SocketChannel channel = server.accept();
                   // Get the socket associated with this channel
                   Socket clientInfo = channel.socket();
                   // to send immediately
    
                   if (!clients.containsKey(clientInfo.getInetAddress())) {
                        Voie voie = new Voie(clientInfo.getInetAddress());
                        if (clientInfo.getLocalPort() == PORT_PRIO) {
                             voie.setKeyPrio(selectedKey);
                             voie.setChannelPrio(channel);
                             synchronized (ServeurNonBloquant.clients) {
                                  clients.put(clientInfo.getInetAddress(), voie);
                             }
                        } else {
                             voie.setKeyNPrio(selectedKey);
                             voie.setChannelNPrio(channel);
                             synchronized (ServeurNonBloquant.clients) {
                                  clients.put(clientInfo.getInetAddress(), voie);
                             }
                        }
                   } else {
                        Voie voie = clients.get(clientInfo.getInetAddress());
                        if (clientInfo.getLocalPort() == PORT_PRIO)
                             voie.setKeyPrio(selectedKey);
                        else
                             voie.setKeyPrio(selectedKey);
    
                   }
                   this.pendingData.put(channel, new LinkedList<ByteBuffer>());
                   System.out.println("-- SocketChannel = " + channel
                             + " register SelectionKey.OP_READ");
                   channel.configureBlocking(false);
                   channel.register(selector, SelectionKey.OP_READ);
                   logger.debug("List of client : " + clients);
              } catch (IOException e) {
                   e.printStackTrace();
              }
         }
         private void write(SelectionKey key) throws IOException {
              SocketChannel socketChannel = (SocketChannel) key.channel();
    
              synchronized (this.pendingData) {
                   Queue<ByteBuffer> queue = (LinkedList<ByteBuffer>) this.pendingData
                             .get(socketChannel);
                   nbrWrites++;
                   // Write until there's not more data ...
                   while (!queue.isEmpty()) {
                        ByteBuffer buf = (ByteBuffer) queue.remove();
                        socketChannel.write(buf);
                   }
                   key.interestOps(SelectionKey.OP_READ);
                   selector.wakeup();
              }
         }
    i'm using only two threads,

    one process data sent by read() and who can send back data to client by calling send method (above on first post) or disconnect clients
    and one timer who uses TimerTask to check if clients steel alive(client send life message each 30 seconds) and disconnect client if no life message received.

    Question : now i can't get rid of pendingData queue, so if i closed channels from any thread, and the data isn't processed yet, the message pending will not be send ?

    after removing the closeRequests queue, i can now disconnect and its better, but in second connection , client connect and send message without any events on server. no accept(), no read() how can this be possible?

    Note after client disconnection from the timer, i got alwayes :
    java.nio.channels.ClosedChannelException
         at sun.nio.ch.SocketChannelImpl.ensureReadOpen(Unknown Source)
         at sun.nio.ch.SocketChannelImpl.read(Unknown Source)
         at com.escota.convoi.ServeurNonBloquant.getMessageByMessageSize(ServeurNonBloquant.java:259)
         at com.escota.convoi.ServeurNonBloquant.readMessage(ServeurNonBloquant.java:251)
         at com.escota.convoi.ServeurNonBloquant.startServer(ServeurNonBloquant.java:161)
         at com.escota.convoi.ServeurNonBloquant.main(ServeurNonBloquant.java:373)
    Edited by: tmazight on Jul 19, 2012 2:54 PM

    Edited by: tmazight on Jul 19, 2012 3:21 PM
  • 5. Re: NIO server : closing client connections properly
    EJP Guru
    Currently Being Moderated
              Voie voie = clients.get(clientInfo.getInetAddress());
                        if (clientInfo.getLocalPort() == PORT_PRIO)
                             voie.setKeyPrio(selectedKey);
                        else
                             voie.setKeyPrio(selectedKey);
    You have a connection leak here. This block can only be entered if a client from that IP address had already connected, in which case you are here losing the key for that connection, and the logic elsewhere won't do anything for a key that isn't registered in 'clients'. Maybe you should close the old connection, or maybe you need to reconsider your data structures.
  • 6. Re: NIO server : closing client connections properly
    tmazight Newbie
    Currently Being Moderated
    this block is used only when client try to connect on second Port
    Voie voie = clients.get(clientInfo.getInetAddress());
                        if (clientInfo.getLocalPort() == PORT_PRIO)
                             voie.setKeyPrio(selectedKey);
                        else
                             voie.setKeyNPrio(selectedKey);
    for test i'm using only one port, and from debug I can see that the key and the channel are stored into "clients"

    i'm getting ClosedChannelException at the step of
     voie.getChannelPrio().close() 
    with Javadoc, the say that :
    Checked exception thrown when an attempt is made to invoke or complete an I/O operation upon channel that is closed, or at least closed to that operation. That this exception is thrown does not necessarily imply that the channel is completely closed. A socket channel whose write half has been shut down, for example, may still be open for reading.

    that's probably what causes my second connection fail, the channel still opened,

    what i don't understand is that my client is removed from "clients", client connection is closed and the client thread is stopped, but at second connection the client socket is OK(show connect) without generating any accept at the server side or read() when client send messages.

    this code print channel registred at the screen
    if (voie.getChannelPrio() != null
    
                   && voie.getKeyPrio() != null) {
                        voie.getChannelPrio().close();
                        if(voie.getChannelPrio().isOpen()){
                             System.out.println("channel opened");
                        }
                        if(voie.getChannelPrio().isRegistered()){
                             System.out.println("channel registred");
                        }
                   }
    Edited by: tmazight on Jul 20, 2012 10:09 AM
  • 7. Re: NIO server : closing client connections properly
    tmazight Newbie
    Currently Being Moderated
    I can now make few successive connections, because with channel.close() my channel was closed but still registered in the selector, with a selector.wakeup() and the end of closeConnection() the channel was closed and deleted from selector registrations.
    but still have ClosedChannelException,

    Thanks EJP
  • 8. Re: NIO server : closing client connections properly
    tmazight Newbie
    Currently Being Moderated
    Now i have problem with sending message and disconnecting after,

    the connection is closed first and my message which is in pendingData is lost.

    How can fix that and keep my pendingData queue (because i don't see how can i send messages otherway).
  • 9. Re: NIO server : closing client connections properly
    gimbal2 Guru
    Currently Being Moderated
    That's a new question - you might want to create a new thread for it and be sure to post only information and code relevant to the new problem.

Legend

  • Correct Answers - 10 points
  • Helpful Answers - 5 points