gUI freezes - Deadlock problem?

[Anders Kviback]

I have an issue with a server - client program that causes deadlocks as I believe. The server class
creates a cleintHandler class and starts a thread there. This clientHandler class
creates MathNumbers that is sent away to the client. The number shows up in the client,(that also has an inner class with a Thread running) that
has a gui, and you're supposed to answer the number, i e putting in the number in the textfield and
clicking on the SendAway Button. The number is sent to the server that calculates the number and
sends back a true or false
The client recieves this boolean and displays if the answer was correct or false. So, this cycle goes
on like that, and you can start up as many clients as you want. The problem is that the client gui
keeps on freezing all the time. The server can send away 2, 3, 4 or 5 numbers but all of a sudden the
window freezes.
I understand that there is this method invokeLater in SwingUtilities but I don't really know how to
apply it here. I don't have Thread implemented in my gui class. The code at the send answer button
looks like this:

public void actionPerformed(ActionEvent ae){
		String text = ae.getActionCommand();
		if(text.equals("Send answer")){
			tal = answerField.getText();			
			icl.sendAnswer(tal);
		} 

Someone said to me that he didn't believe that this was a invokeLater issue. But I don't know, anyone
that has an idea how to implement invokeLater here or if I at all should implement it?

The gui has the EDT and the main thread.

[Steve Luke]

You really do need to be pushing the work out of the GUI thread into another thread - not using invoke later, but using a background thread (like SwingWorker). If you do not know how to do so, read this: http://docs.oracle.com/javase/tutorial/uiswing/concurrency/

That said, that will make your app more responsive, and will keep your application from freezing, but may not fix the problem - instead of freezing the client may just never get a response. To fix the problem you need to know where the hangup is. It could be in the server, it could be in the client, but it is impossible to say with the code you posted. Use a debugger on both the client and server, or use print statements on both sides to see where things get when the application hangs. Once you know where the problem is, fixing it gets much easier.

[K. Tsang]

When the client send numbers to the server, is it asynchronous or synchronous (client waits till server return)?

The send number to server feature should be done in another thread. When the thread returns something from the server, the GUI thread gets notified and inform the user.

[Anders Kviback]

Thanks for answering,

When the client send numbers to the server, is it asynchronous or synchronous (client waits till server return)?

It is synchronous, i e it waits till server return.

This is how I've done it. And it is supposed to be correct.
MathClient class:

try{
					talC = (MatteTalNy)in.readObject();
					String tal = talC.toString();
					showNbr(tal + " = ");
					b = in.readBoolean();//The code stops here and waits
}

The send number to server feature should be done in another thread.

MathClient class has it's own Thread. The server has another Thread. Or is this what you ment?

or use print statements on both sides to see where things get when the application hangs

I will start to look into this and do some System.out.println(); on various places and see if I can find anything.



Thanks for answering.// Anders

[K. Tsang]

Since you mentioned synchronous, then you should also check whether it is the client (GUI) waiting for the server return that "tends" to freeze (maybe server side) or nothing to do with the server code.

[Anders Kviback]

Since you mentioned synchronous, then you should also check whether it is the client (GUI) waiting for the server return that "tends" to freeze (maybe server side) or nothing to do with the server code.

How do I check that?


I have been changing around the code a bit now but then I do get other problems. I have a server class, clienthandler class and a client class(+gui for client).

Server:
Here I have the Serversocket and I start up the client handlers with the code:

new MathClientHandler(s, this).start();

All the streaming at the server side is taken care of in the client handler class, i e

The server crates a new MathNumber and sends this (object) to the client

out.writeObject(new MatteTalNy());
			out.flush();

Then the client recieves the object, presents it to the user and waits for the user to put in the answer.

talC = (MatteTalNy)in.readObject();
	String tal = talC.toString();
	showNbr(tal + " = ");
b = in.readBoolean();

The user puts in the answer which is stored in the object and the client sends it back to the server

	talC.setAnswer(tal);
		try{
		out.writeObject(talC);
		out.flush();

The server recieves the mathnumber(that now contains the answer from the user )

			try{
			
				talSvar = (MatteTalNy) in.readObject();

The server calculates the answer and sees if it was false or true and sends that back to the cleint

out.writeBoolean(answer);
out.flush(); 

The client recieves the result(a boolean) and presents it in a suitable way for the user.

				if(b ==true){
					gui.clear();
					show("Correct answer\n");

My question here is; on the server side , should it all be handled in the client handler class or should part of this be done in the server, (the first sending of a new mathnumber)?

//Anders

[Steve Luke]

It is still unclear to me. Can you provide the server code?

Have you done the print statements as you have been asked? Put a System.out.println() after EVERY line on both the server and the client. Put key information (such as line of code, value of key variables, state of sockets, whatever seems important or informative as a result of the previous line of code or necessary for the next line of code). Then look through the output to see what you get.

[Anders Kviback]

I have done the print statements and it helps out a bit but not the whole way. (what do you mean with state of sockets?)

Here is the server class:

public class  MathServerNy {
	// The socket for communication
	private MatteTalNy talSvar;
	private int portMath;
	private int portFile;
		
	public MathServerNy(int portMath, int portFile){
		this.portMath = portMath;
		this.portFile = portFile;
		new FileClientHandler(portFile).start();
		
		connectToMathClientNy();
		
	}

	@SuppressWarnings("resource")
	public void connectToMathClientNy(){
		
		ServerSocket ssM = null;
		Socket s = null;
		
		//The server starts and listens after clients that connects
		try {
			ssM = new ServerSocket(portMath);
			System.out.println("MathServerNy-Listens on port: " + portMath);
		}
		catch(IOException ioe){
			System.out.println("MathServerNy-Couldn't listen on port: " + portMath);
			return;
		}
		
		while(true){
			try{
				// Wait for someone to connect
				s = ssM.accept();
				
				// A client has connected so we create a new client handler
				System.out.println("\nMathServerNy-MathClientNy ansluten med adress: " + s.getInetAddress().getHostAddress());
				new MathClientHandlerNy(s, this).start();
			}
catch(IOException ioe)
			{
				System.out.println("Fel vid upprättande av förbindelse till klient:\n" + ioe);
				break;
			}
		}//End-while 
	}

The client handler:

public class MathClientHandlerNy extends Thread{

	// The socket for communication
	private MathServerNy msn;
	private Socket socket;
	private int number1;
	private int number2;
	private String operand;
	private int kSvar;
	// The client address
	private String address;
	private MatteTalNy talSvar;
	// Declares out going stream and ingoing stream
	private	ObjectInputStream in;
	private	ObjectOutputStream out;
	
// Constructor
	public MathClientHandlerNy(Socket s, MathServerNy msn)
	{
		socket = s;
		address = socket.getInetAddress().getHostAddress();
		this.msn = msn;
	}

	//Start a thread
	public void run()
	{
		try
		{
			// Creates in and out streams for communication
			out = new ObjectOutputStream(socket.getOutputStream());
			in = new ObjectInputStream(socket.getInputStream());
		}
		catch (IOException ioe)
		{
			System.out.println("MathClientHandlerNy:" + address + ": Unable to create a stream");
			return; // Should close streams and sockets 
		}
		
		try {
			
			out.writeObject(new MatteTalNy());
			out.flush();
		} 
		catch (IOException e) {
			// TODO Auto-generated catch block
			e.printStackTrace();
			return;
		}
		// We do now have a connection with the client
		// It is time to recieve the number that we are supposed to calculate
		while(true){

			//The server recieves the mathNumber(that now consists of the user answer) 
			try{
				talSvar = (MatteTalNy) in.readObject();
			}
			catch (SocketException e){
				System.out.println(address + ": Connection closed");
			}
			catch (ClassNotFoundException e) {
				// TODO Auto-generated catch block
				e.printStackTrace();
			} 
			catch (IOException e) {
				// TODO Auto-generated catch block
				e.printStackTrace();
			}	
		
			try{
				
				String klientSvar = talSvar.getAnswer();
				kSvar = Integer.parseInt(klientSvar);
				number1 = talSvar.getNumber1();
				number2 = talSvar.getNumber2();
				operand = talSvar.getOperand(); 

				MatteTalNy tal2 = new MatteTalNy(number1,number2,operand);
				int serverCalc = tal2.calculate();
				if (serverCalc == kSvar){
					boolean answer = true;
					out.writeBoolean(answer);
					out.flush(); 
				}
				else {
					boolean answer = false;
					out.writeBoolean(answer);
					out.flush();
				}
			}
			catch (SocketException se){
				System.out.println(address + ": Nedkopplad");
				break;
			}
			catch(NullPointerException ne){
				System.out.println("MathClientHandlerNy: Klienten har inte angett något tal");
				break;
			}
			catch(NumberFormatException ne){
				System.out.println("Klienten har inte angett något tal");
				break;
			}
			catch(ArithmeticException ae){
				System.out.println("Division med noll!\nVänta på nytt tal!\n");
				break;
			}
			catch (IOException e) {
				// TODO Auto-generated catch block
				e.printStackTrace();
				break;
			}
			
			// Sleeps for a while
			try
			{
				sleep(1000);
			}
			catch (InterruptedException ie)
			{
				break;				
			}

		}//end-while

		// Closing streams and socket
		try
		{
			in.close();
			out.close();
			socket.close();
			System.out.println( address + " MathClientHandler: Connection closed");
		}
		catch (SocketException e){
			// TODO Auto-generated catch block
			System.out.println(address + " Couldn't close connection or sockets/streams");
			return;
		}
		catch(IOException ioe)
		{
			System.out.println(address + " Couldn't close connection or sockets/streams");
			return;
		}
	}//end-run
}//end-class

Are there to many streams in and out of the client handler, or is it not possible to build it that way, i e that I put the first stream, creating the mathnumber, in the server not the clienthandler and sending it from the server to the client instead of sending it from the clienthandler?

Anders

[Steve Luke]

Anders Kviback wrote:I have done the print statements and it helps out a bit but not the whole way. (what do you mean with state of sockets?)

What do you mean by a bit but not the whole way? Which line of code did the Server get to? When you got to the out.writeBoolean(answer) line, what was the value of answer, what was the value or serverCalc, what was the value of kSvar? A Socket has some methods which will tell you if it is bound, connected, closed, or if its streams are shut down. Look at the API, then use those results in the println statements.

[Steve Luke]

A few comments on the code:

1) Your code allows the socket and its input streams and output streams to stay connected and open even if an exception occurs and you abandon the Thread. This is horrible, and will lead to resource leaks.
2) Your code also allows exception to occur, get reported, then the application to continue as if nothing was wrong. If you didn't fix the problem, then the resources should be cleaned up and the thread stopped, and probably some nasty and informative information posted to the error stream about the problem. But you should not let execution to continue.
3) This code:

				int serverCalc = tal2.calculate();
				if (serverCalc == kSvar){
					boolean answer = true;
					out.writeBoolean(answer);
					out.flush(); 
				}
				else {
					boolean answer = false;
					out.writeBoolean(answer);
					out.flush();
				}

Should be written as:

				int serverCalc = tal2.calculate();
				boolean answer = (serverCalc == kSvar);
				out.writeBoolean(answer);
				out.flush(); 

You will be less likely to introduce bugs by having less code that does the same work. See if you can find other places in your code that can be equally reduced.

[Anders Kviback]

Thanks alot here.
This piece of code

Should be written as:

int serverCalc = tal2.calculate();
boolean answer = (serverCalc == kSvar);
out.writeBoolean(answer);
out.flush();

really helped me. It all started to work out much better. I have System.out.println on 15 different places around the code. It does help. But now I have another thing. When I get division by zero I have to deal with that in some way.
Where should I take care of it. Should I take care of it in the beginning were the numbers are created, i e in the class MathNumber.?

The constructor in MathNumber that creates the number looks like this:

public MathNumber(){
		
		this.number1 = (int)(Math.random()*19);
		this.number2 = (int)(Math.random()*19);

		int oand = (int)(Math.random()*5);

		switch(oand){
		case 0: 
			this.operand = "/";
			break;
		case 1: 
			this.operand = "-";
			break;
		case 2: 
			this.operand = "*";
			break;
		case 3: 
			this.operand = "+";
break;
		default:
			this.operand = "+";
			}
	}

Isn't that the best way to deal with it? I mean, somewhere here

case 0 :
if (this.number2==0)
doSomething();//????but what?
else 
this.operand = "/";

Instead of waiting with it so that the client has to take care of it. I mean I don't want to send something that is not correct. It will only give me more problem. So it is better to take care of the problem at the source.??
But how do I do that? I have to in some way create a new mathnumber. Now it is created in MathClientHandler, line 40:

try {  
            out.writeObject(new MathNumber());  
            out.flush();  
        }   
        catch (IOException e) {  
            // TODO Auto-generated catch block  
            e.printStackTrace();  
            return;  
        }  

I tried to make a new method out of it; public void sendNumber() and then I changed the contructor in MathNumber to MathNumber(this)

But it didn't work out. Could you just maybe give me a hint about how to deal with it?

Anders

[Steve Luke]

Personally, I would handle it in both the server and the client. I would handle it in the client because it is the quickest way to repair the problem. I would handle it on the server because the server and client are to separate and independent applications, and what happens if client doesn't get it right, or if a different client (one I didn't write) comes along? Better safe than sorry.

The way I would fix it:

1) At the end of the MathNumber() method I would check if the operator is "/" and if it is, I would check number2 to see if it is 0. If it is, I would generate a new random number for number2.

2) In the MathClientHandlerNy class' run() method, after you read talSvar from the stream, you get number1, number2, and the operantor. At this point would check to see if the operator is "/" and if it is, if number2 is 0. If both true, then I would immediately respond with an error message written to the stream. You should have some protocol in place where your client reads the Stream and checks if the operation was successful, or if there was an error, and if there was an error what was it caused by. For example, instead of just writing the answer to the output stream, write a number: 0 means success, anything else means failure, and specific numbers mean specific types of failure. Then, if there was no failure, you write the boolean which indicates the correct answer or not. The client would first read the integer from the stream, and if it is 0, read the boolean to see if they got the answer correct. If the number is not 0, then they would look up what the response was and react accordingly (fix the numbers, fix the formats, etc...). But one of the key things is that your server should always return some signal to the client so the client knows to continue. So all those catch() clauses should have some response to the client telling them what problem occurred. (note, you don't need to have a code for every exception. Using -1 for 'general server error' could be fine, and you would use it, for example, with the NullPointerException. Also note that there are several exceptions that you can't respond this way with - SocketException, for example, and maybe IOException. I would try anyway, but the issue is moot because the same exception would probably occur on the client side).

A word on names: You have a method named MathNumber(). This is not very good practice. Method names should be actions (because methods do things) and should begin with a lowercase letter. So generateNumbers() is a better name. Using MathNumber is confusing because it is a noun and begins with a capital letter - both of which are used for class names (since classes represent things).

[Anders Kviback]

Thanks a lot again.

I would say that the code almost works perfect now. Now division by zero is fixed as well. great!
But still it freezes occasionaly.

And what I can see from my system.out.println is that it is always at reading the stream, both server and client side.

MathClientHandler:

try{
				System.out.println("6.The server recieves the mathnumber (that now contains the answer from the user) )");
				talSvar = (MatteTalNy) in.readObject();
			}

MathClient:

try{
					System.out.println("4.The client recieves the object, presents the mathnumber to the user and waits for the user to put in his answer");
					talC = (MatteTalNy)in.readObject();
					String tal = talC.toString();
					showNbr(tal + " = ");
					b = in.readBoolean();
				}

'
System.out.println("9. The client recieves the result (a boolean) and presents it in a suitble manner for the user");
				if(b ==true){
					gui.clear();
					show("Correct answer\n");
				}
				else if(b==false){
					gui.clear();
					show("Wrong answer\n");
				}
				else show("You have to give a number!\n");

So, it stops at some of these points. Why?


MathNumber is not a method. It is a constructor. That's why it starts with a capital letter.


Thanks a lot again for helping me out here

// Anders

[Steve Luke]

Anders Kviback wrote:... it freezes occasionaly.
...
So, it stops at some of these points. Why?

Unknown. You have to put system outs before every line and after every line to see exactly where it goes. and that includes catch() clauses. My guess is that you are still getting some exception in the server, not handling it well, and not sending a response to the client that an error occurred. So the client waits for a response and doesn't get it.

Anders Kviback wrote:MathNumber is not a method. It is a constructor. That's why it starts with a capital letter.

Ahh, my apologies, I should have realized it had no return type. It makes sense now


Another point on code clarity, though. You have this:

			if(b ==true){
					gui.clear();
					show("Correct answer\n");
				}
				else if(b==false){
					gui.clear();
					show("Wrong answer\n");
				}
				else show("You have to give a number!\n");

First, you should not compare a boolean against true or false, it is too easy to make a mistake (instead of if (b == true) use if (b), to avoid the occasional if (b = true) mistake, which is valid syntax but not what you want).
Second, you don't need to make the negative boolean comparison (else if (b == false) or else if (!b), because if it ever gets to the else clause then the if part must be true - so it is wasted code.
Finally, the lase clause (else without the if) is wasted code. How could the code ever reach there? Either b is true or b is false, there is no other option, a boolean only has two states. This even works with Booleans (capital B) as long as you make getting a null response impossible.

So I would write it as (remember, don't duplicate code!):

    gui.clear();
    if (b) {
        show("Correct answer\n");
    } else {
        show("Wrong answer\n");
    }

[Steve Luke]

Steve Luke wrote:Unknown. You have to put system outs before every line and after every line to see exactly where it goes. and that includes catch() clauses. My guess is that you are still getting some exception in the server, not handling it well, and not sending a response to the client that an error occurred. So the client waits for a response and doesn't get it.

Well, maybe not every line, you could narrow it down sequentially to the point where you now exactly which branch through the method(s) where taken and which line was last started, but not ended. You can do this by putting fewer system outs to narrow down to some section of code, then add more system outs to trace the paths through that section, then do line-by-line system outs to get precise line causing the problem.

You could also do the same thing by using a debugger and scattering healthy doses of breakpoints throughout the code. Some IDEs will even let you add new breakpoints and remove others during execution, so you can put enough of them in to see what path through the code is being executed, then doing line-by-line through the problem section.