2

I've been working on a NIO-based chat application of quite trivial logic: any message sent by any client should be visible to the rest of the users.

Right now, I'm sort of in the middle of the work, I've got pretty complete classes of the clients (and their GUI part) and the server but I've stumbled on a problem I couldn't find any solution on anywhere. Namely, if I run an instance of the server and one instance of the client, in my consoles (one for client, one for the server) I see a nice, expected conversation. However, after adding additional client, this newly created client doesn't get responses from the server - the first still has a valid connection.

I'm not thinking about broadcasting messages to all the clients yet, now I'd like to solve the problem of the lack of proper communication between each of my clients and the server since, I think that broadcasting shouldn't be so big a deal if the communication is fine.

I'd like to also add that I've tried many other ways of instantiating the clients: in one thread, firstly instantiating the clients then applying methods on them, I've event tried using invokeLater from SwingUtilities, since that's the proper way to boot up GUI. Sadly, neither worked.

What should I change to achieve proper communication between clients and the server? What am I doing wrong?

This is the log from client console:

Awaiting message from: client2...
Awaiting message from: client1...

after creating the clients - before any action

1 Message: client1 :: simpleMess1
2 started pushing message from: client1
3 Server response on client side: ECHO RESPONSE: client1 :: simpleMess1
4 Message: client2 :: simpleMessage from c2
5 started pushing message from: client2
6 
7 -- No response from client2. AND next try from client2 shows no log at all (!)
8 
9 Message: client1 :: simple mess2 from c1
10 started pushing message from: client1
11 Server response on client side: ECHO RESPONSE: client1 :: simpleMess1

And the log from server side console:

1 Server started...
2   S: Key is acceptable
3   S: Key is acceptable
4
5 -- after creating the clients before any action   
6 S: Key is readable.

The console output clearly shows that the server receives acceptable keys from both clients but it suggest also that only one SocketChannel has a SelectionKey of readable type, but I've got no clue why. Moreover, I think that the order of creating the clients doesn't matter because as I tested: the client that talks properly with the server is always the one that starts communication as first. Below I'm posting my Server and Client classes code, hoping You'll Guys help me sort it out.

Firstly, Server class:

import java.nio.channels.SelectionKey;
import java.nio.channels.Selector;
import java.nio.channels.ServerSocketChannel;
import java.nio.channels.SocketChannel;
import java.io.IOException;
import java.net.InetSocketAddress;
import java.nio.ByteBuffer;
import java.nio.CharBuffer;
import java.nio.charset.Charset;
import java.util.Iterator;
import java.util.Set;

public class Server {

private ServerSocketChannel serverSocketChannel = null;
private Selector selector = null;
private StringBuffer messageResponse = new StringBuffer();

private static Charset charset = Charset.forName("ISO-8859-2");
private static final int BSIZE = 1024;
private ByteBuffer byteBuffer = ByteBuffer.allocate(BSIZE);
private StringBuffer incomingClientMessage = new StringBuffer();
Set<SocketChannel> clientsSet = new HashSet<>();



public Server(String host, int port) {
    try {
        serverSocketChannel = ServerSocketChannel.open();
        serverSocketChannel.configureBlocking(false);
        serverSocketChannel.socket().bind(new InetSocketAddress(host, port));
        selector = Selector.open();
        serverSocketChannel.register(selector, SelectionKey.OP_ACCEPT);
    }
    catch (Exception exc) {
        exc.printStackTrace();
        System.exit(1);
    }
    System.out.println("Server started...");
    serviceConnections();
}

private void serviceConnections() {
    boolean serverIsRunning = true;

    while (serverIsRunning) {
        try {
            selector.select();
            Set keys = selector.selectedKeys();

            Iterator iter = keys.iterator();
            while (iter.hasNext()) {

                SelectionKey key = (SelectionKey) iter.next();
                iter.remove();

                if (key.isAcceptable()) {
                    System.out.println("\tS: Key is acceptable");

                    SocketChannel incomingSocketChannel = serverSocketChannel.accept();
                    incomingSocketChannel.configureBlocking(false);
                    incomingSocketChannel.register(selector, SelectionKey.OP_READ);
                    clientsSet.add(incomingSocketChannel);

                    continue;
                }

                if (key.isReadable()) {
                    System.out.println("\tS: Key is readable.");

                    SocketChannel incomingSocketChannel = (SocketChannel) key.channel();
                    serviceRequest(incomingSocketChannel);
                    continue;
                }
            }
        }
        catch (Exception exc) {
            exc.printStackTrace();
            continue;
        }
    }
}

private void serviceRequest(SocketChannel sc) {
    if (!sc.isOpen()) return;

    incomingClientMessage.setLength(0);
    byteBuffer.clear();
    try {
        while (true) {
            int n = sc.read(byteBuffer);
            if (n > 0) {
                byteBuffer.flip();
                CharBuffer cbuf = charset.decode(byteBuffer);
                while (cbuf.hasRemaining()) {
                    char c = cbuf.get();
                    if (c == '\r' || c == '\n') break;
                    incomingClientMessage.append(c);
                }
            }
            writeResp(sc, "ECHO RESPONSE: " + incomingClientMessage.toString());
        }


    }
    catch (Exception exc) {         
        exc.printStackTrace();
        try {
            sc.close();
            sc.socket().close();
        }
        catch (Exception e) {
        }
    }
}

private void writeResp(SocketChannel sc, String addMsg)
        throws IOException {
    messageResponse.setLength(0);
    messageResponse.append(addMsg);
    messageResponse.append('\n');
    ByteBuffer buf = charset.encode(CharBuffer.wrap(messageResponse));
    sc.write(buf);
}
//second version - with an attempt to acomlish broadcasting
private void writeResp(SocketChannel sc, String addMsg)
        throws IOException {
    messageResponse.setLength(0);
    messageResponse.append(addMsg);
    messageResponse.append('\n');
    ByteBuffer buf = charset.encode(CharBuffer.wrap(messageResponse));

    System.out.println("clientsSet: " + clientsSet.size());
    for (SocketChannel socketChannel : clientsSet) {
        System.out.println("writing to: " + socketChannel.getRemoteAddress());
        socketChannel.write(buf);
        buf.rewind();
    }
}

public static void main(String[] args) {
    try {
        final String HOST = "localhost";
        final int PORT = 5000;
        new Server(HOST, PORT);
    }
    catch (Exception exc) {
        exc.printStackTrace();
        System.exit(1);
    }
}
}

and the Client class:

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.PrintWriter;
import java.net.Socket;
import java.net.UnknownHostException;

public class Client {
private ClientView clientView;
private String hostName;
private int port;
private String clientName;
private Socket socket = null;
private PrintWriter printWriterOUT = null;
private BufferedReader bufferedReaderIN = null;

public Client(String hostName, int port, String clientName) {
    this.hostName = hostName;
    this.port = port;
    this.clientName = clientName;

    initView();
}

public void handleConnection() {

    try {
        socket = new Socket(hostName, port);
        printWriterOUT = new PrintWriter(socket.getOutputStream(), true);
        bufferedReaderIN = new BufferedReader(new InputStreamReader(socket.getInputStream()));

        waitForIncomingMessageFromClientView();


        bufferedReaderIN.close();
        printWriterOUT.close();
        socket.close();

    }
    catch (UnknownHostException e) {
        System.err.println("Unknown host: " + hostName);
        System.exit(2);
    }
    catch (IOException e) {
        System.err.println("I/O err dla");
        System.exit(3);
    }
    catch (Exception exc) {
        exc.printStackTrace();
        System.exit(4);
    }
}


public void initView() {
    clientView = new ClientView(clientName);
}

public void waitForIncomingMessageFromClientView() {
    System.out.println("Awaiting message from: " + clientName + "...");
    while (true) {
        if (clientView.isSent) {
            System.out.println("Message: " + clientView.getOutgoingMessage());
            pushClientViewMessageToServer();
            clientView.setIsSent(false);
        }
    }
}

public void pushClientViewMessageToServer() {
    String clientViewMessage = clientView.getOutgoingMessage();
    System.out.println("started pushing message from: " + clientView.getClientName());

    try {
        printWriterOUT.println(clientViewMessage);
        String resp = bufferedReaderIN.readLine();
        System.out.println("Server response on client side: " + resp);
    }
    catch (IOException e) {
        e.printStackTrace();
    }
}


public static void main(String[] args) {

    Thread thread1 = new Thread(new Runnable() {

        @Override
        public void run() {
            Client c1 = new Client("localhost", 5000, "client1");
            c1.handleConnection();
        }
    });
    thread1.start();

    Thread thread2 = new Thread(new Runnable() {
        @Override
        public void run() {
            Client c2 = new Client("localhost", 5000, "client2");
            c2.handleConnection();
        }
    });
    thread2.start();

}

}

I'll apprecaite any help from You Guys.

EDIT: the second version of writeResp method attempting to broadcast echo to all the clients produces such log:

Server started...
clientsSet: 2
writing to: /127.0.0.1:63666
writing to: /127.0.0.1:63665
clientsSet: 2
writing to: /127.0.0.1:63666
writing to: /127.0.0.1:63665

It seems like there are two clients and I'm wondering why they don't get proper reply from the server.

musztard2000
  • 127
  • 2
  • 14

1 Answers1

1
while (true) {
    int n = sc.read(byteBuffer);
    if (n > 0) {
        byteBuffer.flip();
        CharBuffer cbuf = charset.decode(byteBuffer);
        while (cbuf.hasRemaining()) {
            char c = cbuf.get();
            if (c == '\r' || c == '\n') break;
            incomingClientMessage.append(c);
        }
    }

There is a major problem here. If read() returns -1 you should close the SocketChannel, and if it returns -1 or zero you should break out of the loop.

user207421
  • 305,947
  • 44
  • 307
  • 483
  • Thank U very much, so after the `if` statement I should place an `else` clause closing the channel like this: `sc.close()`, yes? But why should I serve also this condition separately `n<=0` if I have that one: `if (n > 0) {...` ? Could You tell me about the internals of the the logic behind it? – musztard2000 Nov 06 '15 at 08:57
  • Sigh. If `n < 0` the peer has closed the connection and you should do likewise. If `n < 0` *or* `n == 0`, which is expressed as `n <= 0`, there is no reason to continue the loop. Neither is captured by your present code. – user207421 Nov 06 '15 at 09:01
  • Ok, if I get you right: `while (true) { int n = sc.read(byteBuffer); if (n > 0) { byteBuffer.flip(); CharBuffer cbuf = charset.decode(byteBuffer); while (cbuf.hasRemaining()) { char c = cbuf.get(); if (c == '\r' || c == '\n') break; incomingClientMessage.append(c); } } else { /*it executes if n is <=0*/ sc.close(); break; }` should do the work? – musztard2000 Nov 06 '15 at 09:05
  • (1) No. Read what I wrote again. (2) You can see for yourself that code posted in comments is completely illegible. – user207421 Nov 06 '15 at 09:08
  • (2) Sorry fot that I've never done it before - as I see shouldn't do it again. (1) I've read a couple of times and I'm really sorry but I don't get it right: inside the `while (true)` loop I should add two more conditions checking the size of `n` and depending on its value should I either do `sc.close()` or `break;` the `while`? – musztard2000 Nov 06 '15 at 09:15
  • Try again. I expressed myself clearly. I can't help you if you can't implement from clear instructions. It is a requirement of this business. I'm not going to write your code for you. – user207421 Nov 06 '15 at 09:21
  • And I really don't expect You do that. :) After placing two conditions before `if (n >0)` the communication started working. Thank You for patience! – musztard2000 Nov 06 '15 at 09:21
  • I'd like to broadcast one client's message to all the other clients. My idea was to add a `Set` to `Server` class and to add new clients' `socketChannels` inside `if(key.isAcceptable)` condition. Then my plan was to iterate through this set inside `writeResponse` method. It doesn't work properly though. Could you please shed some light on how could it be done? – musztard2000 Nov 06 '15 at 14:15
  • "Doesn't work properly" is not a problem description. – user207421 Nov 06 '15 at 22:12
  • Your're right, after some thoughts I decided to create separate question for it since the header of this one doesn't describe the problem well. Here is a link [Boradcasting messages across all the clients in chat using NIO](http://stackoverflow.com/questions/33570932/boradcasting-messages-across-all-the-clients-in-chat-using-nio?lq=1) under which there is better description of the issue with broadcasting. – musztard2000 Nov 07 '15 at 07:08
  • I've debugged the server of my application and it seems that both client `SocketChannel`s get correct data. But the data seems to be wrongly distributed via buffer inside`Client` class, do You think it might be the reason of this problem? – musztard2000 Nov 07 '15 at 07:57
  • @vladimir1923 I have no idea what you're talking about, and no intention of solving a new question in the commentary to an answer to a different question. If you have a new question, ask it, *as a question,* with *code* and all the other paraphernalia required to get an answer. – user207421 Nov 07 '15 at 09:27
  • that's the reason why I created a new question yesterday. In my comment from 3 hours ago I posted a comment with a link to that separate question... This is a link to that question: [Boradcasting messages across all the clients in chat using NIO](http://stackoverflow.com/questions/33570932/boradcasting-messages-across-all-the-clients-in-chat-using-nio?lq=1). Take a look at it, please. – musztard2000 Nov 07 '15 at 09:35