Re: fhpd vs RuntimeExceptions

[Andrew Cagney <cagney@redhat.com>]

Mark Wielaard wrote:
> Hi,
>
> While investigating some bugs I noticed that the fhpd sometimes swallows
> the RuntimeExceptions from the core (and there I thought all was
> fine...).
Just fyi, broadly the message stuff, at least for normal output, is 
going away.

The reason is that the cli alternates between using addMessage and 
PrintWriter.print(...) for normal out; so I've been migrating stuff to 
just do PrintWriter.print.  But this leaves us still needing a way to 
consistently report errors.
>  This makes debugging the fhpd itself a little hard. I added a
> possible exception cause to the Message class and while I was at it
> added checks to make sure we don't present the user with an empty or
> null message (which is very uninformative). Currently we always print
> these possible exception causes when they are attached to a Message in
> CLI.flushMessages().

We were printing both the error and the stack, that looked terrible (the 
number of times an exception occurs for legitimated reasons is 
surprising); so they were turned off.  Did this turn them back on?

>  But we could add a fhpd 'frysk_debug mode' so they
> are only printed if an 'expert' is running frysk. For now I assume all
> users are experts and want to see them, but maybe it is annoying.
> Opinions?
>
> 2007-11-14  Mark Wielaard  <mwielaard@redhat.com>
>     
>      * CLI.java (addMessage): New variant that takes a possible
>      exception cause.
>      (doAttach): Use new addMessage().
>      (execCommand): Likewise.
>      (flushMessages): Add possible exception cause if present.
>      Actually flush outWriter.
>      * EvalCommands.java (eval): Add possible RuntimeException cause.
>      * Message.java (Message): Add constructor that takes a possible
>      exception cause.
>      (getException): New method.
>      * PlocationCommand.java (interpret): Add possible RuntimeException
>      cause.
>
> All tests pass before and after this patch (go hackers - all PASS!)
>
> Cheers,
>
> Mark
>   
> ------------------------------------------------------------------------
>
> diff --git a/frysk-core/frysk/hpd/CLI.java b/frysk-core/frysk/hpd/CLI.java
> index 3c14b2e..d79e557 100644
> --- a/frysk-core/frysk/hpd/CLI.java
> +++ b/frysk-core/frysk/hpd/CLI.java
> @@ -123,6 +123,7 @@ public class CLI {
>  	    return topLevelCommand.complete(this, new Input(buffer), cursor,
>  					    candidates);
>  	} catch (RuntimeException e) {
> +	    // XXX - FIXME - What if this is something fatal?
>  	    return -1;
>  	}
>      }
> @@ -142,7 +143,7 @@ public class CLI {
>  	    outWriter.print("Attached to process ");
>  	    outWriter.println(attached);
>          } catch (InterruptedException ie) {
> -            addMessage("Attach interrupted.", Message.TYPE_ERROR);
> +            addMessage("Attach interrupted.", Message.TYPE_ERROR, ie);
>              return;
>          } finally {
>              synchronized (this) {
> @@ -253,19 +254,14 @@ public class CLI {
>  		    topLevelCommand.interpret(this, command);
>                  }
>              }
> -            catch (NullPointerException e) {
> -                e.printStackTrace();
> -                String msg = "";
> -                if (e.getMessage() != null)
> -                    msg = e.getMessage();
> -
> -                addMessage(msg, Message.TYPE_DBG_ERROR);
> -            }
> +            catch (InvalidCommandException ice) {
> +                addMessage(ice.getMessage(), Message.TYPE_ERROR);
> +	    }
>              catch (RuntimeException e) {
> -                String msg = "";
> -                if (e.getMessage() != null)
> -                    msg = e.getMessage();
> -                addMessage(msg, Message.TYPE_ERROR);
> +                String msg = e.getMessage();
> +                if (msg == null || msg.equals(""))
> +                    msg = e.toString();
> +                addMessage(msg, Message.TYPE_DBG_ERROR, e);
>              }
>              flushMessages();
>          }
> @@ -280,6 +276,10 @@ public class CLI {
>          addMessage(new Message(msg, type));
>      }
>  
> +    void addMessage(String msg, int type, Throwable exc) {
> +        addMessage(new Message(msg, type, exc));
> +    }
> +
>      private void flushMessages() {
>          for (Iterator iter = messages.iterator(); iter.hasNext();) {
>              Message tempmsg = (Message) iter.next();
> @@ -293,8 +293,12 @@ public class CLI {
>              if (prefix != null)
>                  outWriter.print(prefix);
>              outWriter.println(tempmsg.getMessage());
> +	    Throwable exc = tempmsg.getException();
> +	    if (exc != null)
> +		exc.printStackTrace(outWriter);
>              iter.remove();
>          }
> +	outWriter.flush();
>      }
>  
>      PTSet createSet(String set) {
> diff --git a/frysk-core/frysk/hpd/EvalCommands.java b/frysk-core/frysk/hpd/EvalCommands.java
> index 5c69032..d9679db 100644
> --- a/frysk-core/frysk/hpd/EvalCommands.java
> +++ b/frysk-core/frysk/hpd/EvalCommands.java
> @@ -96,7 +96,10 @@ abstract class EvalCommands extends ParameterizedCommand {
>  	    try {
>  		result = cli.parseValue(task, expression, options.dumpTree);
>  	    } catch (RuntimeException nnfe) {
> -		cli.addMessage(nnfe.getMessage(), Message.TYPE_ERROR);
> +		String msg = nnfe.getMessage();
> +		if (msg == null || msg.equals(""))
> +		    msg = nnfe.toString();
> +		cli.addMessage(msg, Message.TYPE_ERROR, nnfe);
>  		continue;
>  	    }
>  
> diff --git a/frysk-core/frysk/hpd/Message.java b/frysk-core/frysk/hpd/Message.java
> index 933305f..aeb4ed2 100644
> --- a/frysk-core/frysk/hpd/Message.java
> +++ b/frysk-core/frysk/hpd/Message.java
> @@ -49,17 +49,39 @@ class Message
>  	public static int TYPE_NORMAL = 3;
>  	public static int TYPE_VERBOSE = 4;
>  
> -	String msg = null;
> -	int type = 0;
> +	private final String msg;
> +	private final int type;
> +        private final Throwable exc;
>  
> -	public Message (String msg, int type)
> +        /**
> +	 * Creates a new Message with the given message and type
> +	 * and no exception.
> +	 */
> +        public Message (String msg, int type)
> +        {
> +	    this(msg, type, null);
> +	}  
> +        /**
> +	 * Creates a new Message with the given message and type.
> +	 * The message cannot be null or empty. The exception is optional
> +	 * and may be null.
> +	 */
> +        public Message (String msg, int type, Throwable exc)
>  	{
> +	        if (msg == null)
> +		    throw new NullPointerException("null msg");
> +
> +	        if (msg.equals(""))
> +		    throw new IllegalArgumentException("empty msg");
> +
>  		this.msg = msg;
>  
>  		if (type < TYPE_DBG_ERROR || type > TYPE_VERBOSE)
>  			throw new IllegalArgumentException("Debugger message created with illegal type.");
>  		else
>  			this.type = type;
> +
> +		this.exc = exc;
>  	}
>  
>  	public String getMessage()
> @@ -71,4 +93,9 @@ class Message
>  	{
>  		return type;
>  	}
> +
> +        public Throwable getException()
> +        {
> +	        return exc;
> +        }
>  }
> diff --git a/frysk-core/frysk/hpd/PlocationCommand.java b/frysk-core/frysk/hpd/PlocationCommand.java
> index 225cc09..61ced31 100644
> --- a/frysk-core/frysk/hpd/PlocationCommand.java
> +++ b/frysk-core/frysk/hpd/PlocationCommand.java
> @@ -77,7 +77,10 @@ class PlocationCommand extends ParameterizedCommand {
>              try {
>                  result = cli.parseValue(task, sInput);	  
>              } catch (RuntimeException nnfe) {
> -                cli.addMessage(nnfe.getMessage(), Message.TYPE_ERROR);
> +		String msg = nnfe.getMessage();
> +		if (msg == null || msg.equals(""))
> +		    msg = nnfe.toString();
> +                cli.addMessage(msg, Message.TYPE_ERROR, nnfe);
>                  continue;
>              }
>  	    result.getLocation().toPrint(cli.outWriter);
>