looping through clients and sending data, only one receives

[Kova]

Hello again.
I’m almost done with my server - client implementation. I have one thread to read and accept connections, and another that sends data to clients. When connection is accepted, it’s channel is added to clients Set:

public static Set clients_set = Collections.synchronizedSet(new HashSet());

The problem is when more then one client connects, my sender thread is looping through set’s iterator (and I tested, it really is looping through it), only single connection receives data. I’m using windows telnet to test it, server is listening on port 23. First client that connects normally receives test data every second. But when more clients connect they in 80% of cases don’t receive anything, and in other 20% they start to receive data but client that received data before them stops receiving. So there is only one client that receives the data at a time. Can someone tell me why? ???
They all connect to same port (for telnet): 23. Can I have multiple connections on same port? I think so.

[Jeff]

@Kova

They all connect to same port (for telnet): 23. Can I have multiple connections on same port? I think so.

Absolutely. Assumign we are talking TCP, each connection spawns a new socket which is the actual end point for that connection on the server.

Can we see th rest of your code? you problem migth well be somehwere other then the 1 line you showed us.

JK

[Ask_Hjorth_Larsen]

That’s funny. I had the same problem once. It was due to a tiny typo.

Check for typos

[Kova]

Yes, it’s TCP. I didn’t put all the code becouse it’s huge. Soon I’ll post code here, but with less relevant stuff converted into logic.

[Kova]

Here’s basic code how things work. I cuted some stuff like exception handling and so on, these are marked with “…”

partial, SERVER thread:

...
if (selection_key.isAcceptable()) {
    ...
    packet_sender.addToClientsSet(s_channel);
    synchronized (packet_sender) {
        packet_sender.notify();
    }
    ...
}
...

partial, PACKET SENDER thread:

...
public void run() {
    while (ViktorijeServer.network_active) {
        try {
            synchronized (this) {
                wait();
            }
        } catch (InterruptedException e) {
            ...
        }
			
        while (ViktorijeServer.network_active && clients_set.size() > 0) {
            clients_iterator = clients_set.iterator();	

            ViktorijePanel.writeBuffer.clear();
            // ...fill buffer
            ViktorijePanel.writeBuffer.flip();        	

            while (clients_iterator.hasNext()) {
                sc = (SocketChannel)clients_iterator.next();
                if (sc.isOpen()) {
                    try {
                        sc.write(ViktorijePanel.writeBuffer);
                    } catch (IOException e) {
                    ...
                    }
                } else {
                    clients_iterator.remove();
                }
            }
        		        		
            try {
                Thread.sleep(1000);    // sends packets every second
            } catch (InterruptedException e) {
                ...
            }
	
        }
    }

    try {
	sc.close();
    } catch (IOException e) {
        ...
    }
}
...
...
public void addToClientsSet(SocketChannel s_channel) {
    if (!clients_set.contains(s_channel)) {
        clients_set.add(s_channel);
        clients_iterator = clients_set.iterator();
    } else {
        System.out.println("Warning: Set already contains selection key and it is not added to set.");
    }
}
...

[Jeff]

where does s_channel come from?

[Kova]

Here is the whole if statment for accepting connections in server thread:

        if (selection_key.isAcceptable()) {
        	ss_channel = (ServerSocketChannel)selection_key.channel();
        	s_channel = ss_channel.accept();
        	s_channel.configureBlocking(false);
	
        	s_channel.register(selector, SelectionKey.OP_READ);
        	packet_sender.addToClientsSet(s_channel);
        	iterator.remove();

        	System.out.println("Got connection from: " + s_channel); 
        	synchronized (packet_sender) {
       			packet_sender.notify();
		}
       	}

[whome]

I don’t think you use HashSet correctly from two threads. You may run ConcurrentModificationException randomly. You have created a sychronized instance of Set but you still must synchronize it while iterating through entries. See example below where you should use synchronized(client_set) to safe iterator loop.

What I suggest is to minimize synchronization as much as possible. Probably new client connections are not created that often so use copy-on-write type of HashSet. New java.concurrent package should have ready to use implementations. It will quarantee a safe iterator use without synchronization. If you loop connections at constant high rate you want to do it this way !!!

public static Set clients_set = Collections.synchronizedSet(new HashSet());
...
// you must syncrhonize iterator loop here, BUT I DO NOT RECOMMEND
// doing it this way. Use copy-on-write variation of Collection implementation instead.
synchronized(client_set) {
   clients_iterator = clients_set.iterator();
   while (clients_iterator.hasNext()) {
     sc = (SocketChannel)clients_iterator.next();
   ...
   }
}

[Kova]

@whome

I don’t think you use HashSet correctly from two threads. You may run ConcurrentModificationException randomly. You have created a sychronized instance of Set but you still must synchronize it while iterating through entries. See example below where you should use synchronized(client_set) to safe iterator loop.

What I suggest is to minimize synchronization as much as possible. Probably new client connections are not created that often so use copy-on-write type of HashSet. New java.concurrent package should have ready to use implementations. It will quarantee a safe iterator use without synchronization. If you loop connections at constant high rate you want to do it this way !!!

public static Set clients_set = Collections.synchronizedSet(new HashSet());
...
// you must syncrhonize iterator loop here, BUT I DO NOT RECOMMEND
// doing it this way. Use copy-on-write variation of Collection implementation instead.
synchronized(client_set) {
   clients_iterator = clients_set.iterator();
   while (clients_iterator.hasNext()) {
     sc = (SocketChannel)clients_iterator.next();
   ...
   }
}

hmm… I used to ran into ConcurrentModificationException until I synchronized it in constructor. After that and much testing not again.
Here’s javadoc, as I understand this constructor sync can be used instead of sync block:

@JavaDoc of HashSet

Note that this implementation is not synchronized. If multiple threads access a set concurrently, and at least one of the threads modifies the set, it must be synchronized externally. This is typically accomplished by synchronizing on some object that naturally encapsulates the set. If no such object exists, the set should be “wrapped” using the Collections.synchronizedSet method. This is best done at creation time, to prevent accidental unsynchronized access to the HashSet instance:
Set s = Collections.synchronizedSet(new HashSet(…));

Also when I actually add clients to set next time sender thread loops through it it loops correctly, number of items and loops print all fine as it should be. I’ll try what you said anyway, maybe you’re righ (and I really hope so).

[Kova]

tried to synchonized it in block like you wrote, but still the same thing…
when I think of it, there is one aspect I didn’t looked at, and that is telnet client itself. It’s a long shot, but maybe it’s programmed that way only one instance can receive data or something. I think I’ll try with custom made java client and another port.

[whome]

Believe me, you must synchronize clients_set instance during iterator loop. Collections.syncrhonizedXX wrappers just isolate getter and setter methods. But if you take Iterator instance from it you must synchronize it until iterator is completed. But as I suggested avoid synchronization completely and use copy-on-write variation from HashSet collection.

[Kova]

can you tell me how to use copy-on-write HashSet? Never used it before, or concurent package.

[whome]

Look at the JRE1.5 and java.util.concurrent package.
http://java.sun.com/j2se/1.5.0/docs/api/java/util/concurrent/CopyOnWriteArraySet.html