1

Apologies for the ambiguous title - I couldn't think how to word this for a title.

Basically, I'm playing around with creating a simple server/client programs in Java. My server runs a thread to constantly check for new connections, then if a connection is found - it creates a thread and a connection object just for that connection.

Problem comes in at the thread that checks for new connections - it didn't work, until I started error checking and when I added in the line:

System.out.println("");

It suddenly started to work as it should. Removing this line causes no new threads to be added for connections. I assumed it was providing enough delay for the thread to run properly or something, so I put in a for loop for a while to see if that worked - but nothing else has worked apart from that one simple line.

Here's the code for creating new connection threads:

    class ThreadCreator implements Runnable {

    ThreadCreator() {

    }

    public void run() {
        while(true) {
            System.out.println("WHY DOES THIS WORK??");
            for(int i = 0; i < connections.size(); i++) {
                if(connections.get(i).thread == false) {
                    Runnable ReadRunnable = new Read(connections.get(i));
                    Thread ReadThread = new Thread(ReadRunnable, "MWHAHAH");
                    ReadThread.setPriority(Thread.MAX_PRIORITY);
                    ReadThread.start();
                    connections.get(i).thread = true;
                    System.out.println("THREAD CREATED");
                }
            }
        }
    }
}

I can't imagine what could be going wrong..?

Andrew Thompson
  • 168,117
  • 40
  • 217
  • 433
Kevin Pione
  • 299
  • 3
  • 12
  • One possibility is that, without that line, the loop works so fast that it eats all of the available CPU. Check to replace it with an `sleep`. – SJuan76 May 15 '13 at 13:46
  • Your right, I added in a sleep command for the thread and this also works - I don't think it's 'eating all the CPU' - as it connects to only one client – Kevin Pione May 15 '13 at 13:54
  • @KevinWatson, it doesn't matter if it's "only one client." Not having a `Thread.sleep()` statement will cause this `while` loop to execute extremely fast. Try adding a counter inside your `while` loop and you'll see what I mean. – BLuFeNiX May 15 '13 at 14:00
  • I'm aware how fast it will execute, but a while loop wouldn't use up all of the available CPU (and isn't - I checked) – Kevin Pione May 15 '13 at 14:10
  • There's quite a bit wrong with the code... can you show us the declaration of connections? – Jonathan S. Fisher May 15 '13 at 14:26
  • possible duplicate of [Loop doesn't see changed value without a print statement](http://stackoverflow.com/questions/25425130/loop-doesnt-see-changed-value-without-a-print-statement) – Boann Aug 23 '14 at 10:23

2 Answers2

1

One thing to keep in mind is that a System.out.println() almost certainly involves synchronization and a call to wait() somewhere (since println() usually out-flushes and waits for the response to be visible (or at least passed to some other process) until returning.

This can easily "fix" your multi-threaded program if synchronization is the problem.

Joachim Sauer
  • 302,674
  • 57
  • 556
  • 614
  • Putting in Thread.currentThread().sleep(1000) seemed to 'work'. I assume this is still a problem, despite it 'working'? What would this be an indication of? – Kevin Pione May 15 '13 at 13:53
  • It's hard to tell without a lot more detail. It could be that some variable is missing a `volatile` or that you're missing proper synchronization, or you're using non-thread-safe objects, ... – Joachim Sauer May 15 '13 at 13:59
  • I had no idea what volatile meant until you mentioned it. I researched it and realised that the two threads (one listening for messages, one looking for connections) were both accessing the arraylist of connection objects! So I made that arraylist a volatile variable, and it worked! I should probably research into multiple threading a bit more! – Kevin Pione May 15 '13 at 14:09
  • @Kevin Watson, I faced a similar problem in one of my applications which was in production. Could you please share the updated code with "volatile"? – Rajkumar May 15 '13 at 14:22
  • I was trying to post the answer, but I have to have over 10 points or w/e. The updated code is nothing special, I simply changed: public ArrayList connections = new ArrayList(); to: public volatile ArrayList connections = new ArrayList(); It meant that the two threads using the same variable could 'tell' each-other when that variable was updated! Hope that helps! – Kevin Pione May 15 '13 at 14:24
  • @KevinWatson: there's a good chance that you're only hiding the true problem with that. For example, if both threads manipulate the `connections` list, they can easily break it if there's no proper synchronization between them. – Joachim Sauer May 15 '13 at 14:29
  • I was thinking that might be an issue - However, the list isn't actually changed by one of the threads, merely accessed for reading a message from the connection. So only one thread is actually changing the list itself. Although I could easily be wrong here! (In terms of there being no synchronization problems) – Kevin Pione May 15 '13 at 14:33
1

Thanks to Joachim - I managed to fix this problem! The issue was that the thread used for receiving messages from clients and the thread used for looking for new connections were both accessing the same variable (the list of connection).

I did a small amount of research (http://jeremymanson.blogspot.co.uk/2008/11/what-volatile-means-in-java.html) and added the volatile modifier to the variable - which as far as I can see, ensures that any other thread using the object will be notified that it has changed, allowing them to both access it!

It was also mentioned that synchronizing the threads is important, is the variable is being changed by more than one thread. However, my variable was being read by one thread and changed by another, so it seems like the volatile statement was enough here!

Thanks a lot!

Kevin Pione
  • 299
  • 3
  • 12
  • You really should consider using the classes in the java.util.concurrent package instead of creating your own threads. ThreadPoolExecutor maybe: http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ThreadPoolExecutor.html – bobwienholt May 15 '13 at 15:06