In Java, why is my multithreading not working?

Question

At first I did this:

public SpaceCanvas(){
    new Thread(new Runnable () {//this is the thread that triggers updates, no kidding
        int fcount = 0;

        @Override
        public void run() {
                System.out.println("Update thread started!");
                while(!Thread.interrupted()){
                    fcount++;
                    while(players.iterator().hasNext()){
                        players.iterator().next().update(fcount);
                    }
                    while(entities.iterator().hasNext()){
                        entities.iterator().next().update(fcount);
                    }
                    System.out.println("About to paint");
                    repaint();
                    System.out.println("Done with paints");
                    try {
                        Thread.sleep(500);
                    } catch (InterruptedException e) {
                        // TODO Auto-generated catch block
                        e.printStackTrace();
                    }
                }
        }
    }).start();
    players.add(new LocalPlayer(0, 9001, 0, 0, 0, 0, this, null));
}

in the initializer of a thing I call a SpaceCanvas. However, that doesn't allow the canvas, and therefore the applet it is within, to be created, because the Thread doesn't actually run asynchronously. Then, I replaced ".start()" with ".run()" and the thread only ran once, but the SpaceCanvas initialized perfectly.

What did I do wrong, and how do I fix this?

Answer 1

I'm not sure this sort of code works the way you expect it to:

while(players.iterator().hasNext()){
    players.iterator().next().update(fcount);

players.iterator() gets a new iterator for the players collection. If there are 0 items in the collection then it will be false but if there are any items, you will be in an infinite loop, creating a new iterator each time. The iterator() call inside of players generates another new iterator object as well.

I think you should doing something like:

Iterator iterator = players.iterator();
while (iterator.hasNext()) {
    iterator.next().update(fcount);
}

This is the same with your entities loop as well. A better pattern (as of Java 5) is to use the for loop:

for (Player player : players) {
    player.update(fcount);
}

Also, if multiple threads are accessing these collections, they have to be somehow synchronized. You can either use a concurrent collection or you have to make sure every access (read and write) is within a synchronized block.

synchronized (players) {
    for (Player player : players) {
        player.update(fcount);
    }
}
...
// down in the outer thread
synchronized (players) {
    players.add(new LocalPlayer(0, 9001, 0, 0, 0, 0, this, null));
}

Obviously the entities will need to be synchronized in the same manner.