Server sends, client receives but doesn't send back

[Pobunjenik]

Hi,
so I've got this multithreaded server which correctly waits for clients and assigns them to a worker thread. From there, the client is sent an object Packet which contains variables Player and Round (simplified, it contains more data but these are the most important).
I'm building a simple top trumps game and I need two enable two players to play it via LAN.

My idea is to send a Packet object back and forth. The apps would populate the GUI based on the packet's contents.
Threads decide weather to send or receive based on Round:
This is being run in a threads on both the server and the client (+ some house-keeping code):

private void whileChatting() throws IOException { // Allows communication.        
        do {
            if (this.packet.getRound() % 2 == 0) {
                this.packet = receivePacket();
            } else if (this.packet == null || this.packet.getRound() % 2 != 0) {
                sendPacket();
            }
            displayCardImage(this.packet.getP2().getPlayerDeck().peek(), this.cardHand_L);
            displayCardImage(this.packet.getOnTable(), this.cardTable_L);
            displayDeckSizes(this.handDeckSize_L, this.packet.getP2().getPlayerDeck().size());
            displayDeckSizes(this.opponentDeckSize_L, this.packet.getP1().getPlayerDeck().size());
            displayRoundNumber(this.roundNumber_L, this.packet.getRound());
            this.stop = true;
        } while (!this.stop);
    }

Flush is being called in each sendPacket();.
The problem starts when I try to send the packet back! The client properly receives it, does something to it but then refuses to send it back (send and receive methods have System.out "I sent this" or "I got this").

When I click a button in the client, it unpacks the packet in a local variables which are used to generate a new packet and then the new packet is passeds into a new thread (which, in theory, should send it to the server since I flagged setSend(True)):

this.c.setPacket(new Packet(this.round, this.p1, this.p2, this.tableCard));
this.c.setStop(false);
this.c.setSend(true);
Thread connectionThread = new Thread(this.c);
connectionThread.start();

But in reality the new connectionThread never even reaches sendPacket();.

I've been trying all sorts of crazy things for 3 hours now. What am I not understanding?

[JamesCherrill]

That toggled loop looks to me like you are trying to go "procedural" where you should be event driven.
Normally in both client and server you would have a loop that waits for an inbound messages and handles them (updates game state and/or GIU).
In the client, GUI events (OK button etc) trigger sending an outbound message to the server.
In the server an inbound message causes a state update for the game that then sends an update to the client(s).

You enforce clients taking turns etc by enabling/disabling the appropriate GUI components based on the latest state of the game.

ps: If you have finished with your previous topic please mark it "solved"

[Pobunjenik]

If I understadn you correctly, you propose that clicking a button in the client GUI, the event of clicking should call the sendPacket(); method? I tried simply calling sendPacket(); in the client's main, but that freezes up the client's GUI.
Also, class Client implements Runnable and is used like so:
once the player clicks "start game" (a button), client looks for server and connects. Right away, client receives packet 1 from server and presents it's contents in the client GUI.

Clicking "start game" runs this:

                this.c.setJta(this.console_TA);
                this.c.setCardHand_L(this.cardHand_L);
                this.c.setCardTable_L(this.cardTable_L);
                this.c.setOpponentDeckSize_L(this.opponentDeckSize_L);
                this.c.setHandDeckSize_L(this.handDeckSize_L);
                this.c.setRoundNumber_L(this.roundNumber_L);
                this.c.setPort(Integer.parseInt(this.port_TF.getText()));
                this.c.setServerIP(this.serverIp_TF.getText());
                this.connectionThread = new Thread(this.c);
                this.connectionThread.start();

where this.c is an instance of class Client in the GUI (swing GUI btw).
Since Client implements Runnable, it's run method looks like this:

@Override
    public void run() {
        synchronized (this) {
            startRunning();
            try {
                wait();
            } catch (InterruptedException ex) {
                Logger.getLogger(Client.class.getName()).log(Level.SEVERE, null, ex);
            }
        }
    }

I hope to make connectionThread wait until the game is done modifying the Packet object and Client class.

After the player presses "play round", this should happen

    this.p = this.c.getPacket(); // unpacks received Packet
        this.currentRound = this.round;

    // DOES GAME STUFF  

    synchronized (this.c) {
            // inserts new game data into a new packet
            this.p = new Packet(this.round, this.p1, this.p2, this.tableCard);
            // inserts new packet into client
            this.c.setPacket(this.p);
            // tells client it should run while again
            this.c.setStop(false);
            // tells client its time to send, not receive
            this.c.setSend(true);
            if (this.currentRound < this.p.getRound()) {
                this.c.notify();
            }
        }

Also, this is how class Client sends/receives:

private void whileChatting() throws IOException { // Allows communication.
        System.out.println("Send: " + this.send + "; Stop: " + this.stop);
        do {            
            if (this.packet == null || !this.send) { // null means receive first packet
                this.packet = receivePacket();
            }
            if (this.send) {
                sendPacket();
            }
            System.out.println("Packet check: " + this.packet.getRound());
            displayCardImage(this.packet.getP2().getPlayerDeck().peek(), this.cardHand_L);
            displayCardImage(this.packet.getOnTable(), this.cardTable_L);
            displayDeckSizes(this.handDeckSize_L, this.packet.getP2().getPlayerDeck().size());
            displayDeckSizes(this.opponentDeckSize_L, this.packet.getP1().getPlayerDeck().size());
            displayRoundNumber(this.roundNumber_L, this.packet.getRound());
            this.stop = true;
        } while (!stop);
        System.out.println(this.packet.getRound() + " Send: " + this.send + "; Stop: " + this.stop);
    }

[Pobunjenik]

So in short, my problem is modifying booleans send and stop. Something doesn't allow me to change their values. Enabling me to change them as need be would mean my project is done!

[JamesCherrill]

No, the problem is here:

void whileChatting()  { // 
        do {            
            if (this.packet == null || !this.send) { // null means receive first packet
                this.packet = receivePacket();
            }
            if (this.send) {
                sendPacket();
            }
            ...

A loop like that just should not exist.
You should have a thread that connects to the server then loops waiting for and processing input from the conection. In general it does not send. It doesn't depend on whose move it is. It must NOT be the Swing EDT.
You send when the user clicks the OK button or equivalent.

Ihave no idea what the intent of your run method is. If you call that on the EDT you will just hang the GUI, in any other case it will just hang around doing nothing. Why?

[Pobunjenik]

I see, but my OK button is in ClientGUI.java, the sendPacket(); is in Client.java (which implements Runnable; which is passed into a new Thread in ClientGUI.java).

I've researched synchronized and implemented it (along with wait and notify).
Right now, ServerGUI correctly builds a Packet, sends it to the client who correctly accepts and displays it. Player 2 hits "play hand" (ie. the OK button) and ClientGUI.java modifies the Packet and sends it back to ServerGUI.java.

Is the following correct:
1. ObjectInputStream.readObject(); tells the thread to wait until something arrives
2. This code is ok and should work in the server:

synchronized (this) {
            do {
                System.out.println("Send: " + this.send);  // reads boolean of send                  
                if (this.send) { // if true, send a packet
                    sendPacket();
                    this.send = false; // don't send anymore
                }                
                System.out.println("Packet check: " + this.packet.getRound()); // debugging
                // these use SwingUtilities.InvokeLater() to update the GUI
                displayCardImage(this.packet.getP2().getPlayerDeck().peek(), this.cardHand_L);
                displayCardImage(this.packet.getOnTable(), this.cardTable_L);
                displayDeckSizes(this.handDeckSize_L, this.packet.getP2().getPlayerDeck().size());
                displayDeckSizes(this.opponentDeckSize_L, this.packet.getP1().getPlayerDeck().size());
                displayRoundNumber(this.roundNumber_L, this.packet.getRound());

                this.packet = receivePacket(); // wait for new packet from client
            } while (true);
        }

In English, this means listen for new Packets unless send = true;

[JamesCherrill]

Why do you have that test? Why not just listen for packets and process them whenever they arrive?

[Pobunjenik]

Because at some point in the game I need to send a packet from the client's connection thread to the server, thus I need to call sendPacket(); and calling that in ClientGUI.java causes the GUI to freeze up (since it would be called in EDT).

I have found that this does exactly what I want:

private void whileChatting() throws IOException, InterruptedException { // Allows communication.        
        synchronized (this) {
            do {
                if (!this.send) {
                    this.packet = receivePacket();
                    System.out.println("Packet check: " + this.packet.getRound());
                    displayCardImage(this.packet.getP2().getPlayerDeck().peek(), this.cardHand_L);
                    displayCardImage(this.packet.getOnTable(), this.cardTable_L);
                    displayDeckSizes(this.handDeckSize_L, this.packet.getP2().getPlayerDeck().size());
                    displayDeckSizes(this.opponentDeckSize_L, this.packet.getP1().getPlayerDeck().size());
                    displayRoundNumber(this.roundNumber_L, this.packet.getRound());
                    showMessage("Your turn!");
                    setButton(this.jb, true);
                }                
                wait();
                if (this.send) {
                    sendPacket();
                    displayCardImage(this.packet.getP2().getPlayerDeck().peek(), this.cardHand_L);
                    displayCardImage(this.packet.getOnTable(), this.cardTable_L);
                    displayDeckSizes(this.handDeckSize_L, this.packet.getP2().getPlayerDeck().size());
                    displayDeckSizes(this.opponentDeckSize_L, this.packet.getP1().getPlayerDeck().size());
                    displayRoundNumber(this.roundNumber_L, this.packet.getRound());
                    this.send = false;
                    setButton(this.jb, false);
                    showMessage("Waiting for player 1...");
                }                
            } while (true);
        }
    }

[JamesCherrill]

Sending a packet should not take more than a millisecond or two, so there's no problem doing that on the EDT. Even if that was a problem you could just delegate the send to a worker thread. Either way there's no need for an infinite loop that waits for things to send.

This tiny (160 lines total!) runnable example server and client shows what I mean. It sends text back and forth, but Objects are just a different type of Stream. Ps The client has a thread for sending because it uses the system console and has to wait for user input. If you have a GUI then the Swing EDT will perform the same function...

import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.OutputStreamWriter;
import java.io.PrintWriter;
import java.net.InetSocketAddress;
import java.net.ServerSocket;
import java.net.Socket;

public class SimplestServer implements Runnable {
   // simple demo of server for multiple clients (eg chat room)
   // sends "hello" message then echoes any input from client
   //
   // NB: This demo has no real error handling or shutdown/cleanup logic - 
   // you have to crash both client and server to stop it.
   // a real application will need an appropriate solid implementation

   public static void main(String[] args) {
      new SimplestServer(999).start();

      // wait a bit, then start a client...
      try {
         Thread.sleep(1000); // allow server to start
      } catch (InterruptedException e) {
      }
      new SimplestClient("localhost", 999).start();
   }

   private ServerSocket serverSocket;

   public SimplestServer(int port) {
      try { // Create the server socket
         serverSocket = new ServerSocket(port);
      } catch (IOException e) {
         System.out.println("Server couldn't open listen socket on port " + port + "\n" + e);
      }
   }

   public void start() {
      new Thread(this).start();
   }

   @Override
   public void run() {
      try {
         System.out.println("Server is listening");
         while (true) {
            Socket clientSocket = serverSocket.accept();
            System.out.println("Connect from: " + clientSocket);
            new Thread(new ClientInterface(clientSocket)).start();
         }
      } catch (IOException e) {
         e.printStackTrace();
      }
      // shutdown and tidy-up logic (logoff sockets etc) omitted for clarity
   }

   private class ClientInterface implements Runnable {
      // starts a thread to wait for and process input from the client
      // also provides a sendOutput method to send stuff to the client any time

      private final BufferedReader inbound; 
      private final PrintWriter outbound;

      public ClientInterface(Socket clientSocket) throws IOException {
         inbound = new BufferedReader(new InputStreamReader(
                 clientSocket.getInputStream()));
         outbound = new PrintWriter(new BufferedWriter(new OutputStreamWriter(
                 clientSocket.getOutputStream())), true); // autoflush
         outbound.println("You are now connected to the server");
      }

      @Override
      public void run() {
         try {
            String input;
            while ((input = inbound.readLine()) != null) {
               System.out.println("Server received " + input);
               sendOutput("Echo: " + input); // echo to client
            }
         } catch (IOException e) {
            e.printStackTrace();
         }
         // shutdown and tidy-up logic (logoff streams etc) omitted for clarity
      }

      public void sendOutput(String output) {
         // (can be called any time, not only in response to input from client)
         System.out.println("Server is sending " + output);
         outbound.println(output);
      }
   }
}

class SimplestClient {
   // connects to the server and starts two threads:
   // one waits for input from the server and prints it
   // the other waits for input from the user and sends it to the server

   private BufferedReader inbound;
   private PrintWriter outbound;

   public SimplestClient(String host, int port) {
      try {
         InetSocketAddress addr = new InetSocketAddress(host, port);
         Socket socket = new Socket();
         socket.connect(addr, 2000);
         inbound = new BufferedReader(new InputStreamReader(
                 socket.getInputStream()));
         outbound = new PrintWriter(new BufferedWriter(new OutputStreamWriter(
                 socket.getOutputStream())), true); // autoflush
      } catch (IOException e) {
         e.printStackTrace();
      }
   }

   public void start() {
         new Thread(new ReceiveLoop()).start();
         new Thread(new SendLoop()).start();
   }

   private class ReceiveLoop implements Runnable {
      // just prints whatever it receives from the server

      @Override
      public void run() {
         String input;
         try {
            while ((input = inbound.readLine()) != null) {
               System.out.println("  Client received " + input);
            }
         } catch (IOException e) {
            e.printStackTrace();
         }
      }
   }

   private class SendLoop implements Runnable {
      // just sends user input to the server

      @Override
      public void run() {
         BufferedReader console = new BufferedReader(new InputStreamReader(
                 System.in));
         String input;
         try {
            while ((input = console.readLine()) != null) {
               System.out.println("  Client sending " + input);
               outbound.println(input);
            }
         } catch (IOException e) {
            e.printStackTrace();
         }
      }
   }

   // shutdown and tidy-up logic (logoff streams/sockets etc) omitted for clarity
}