10 Replies Latest reply on Feb 18, 2009 6:56 PM by David Lloyd

    Those terrible error messages

    David Lloyd Master

      One of the most off-putting things about JBossMC is the confusing, non-human error messages it produces when a deployment fails. Carlo's thread gave me the notion to try and fix this. Here's the patch to AbstractKernelDeployer which humanizes the error message. What do you think? Should I commit it?

      Index: kernel/src/main/java/org/jboss/kernel/plugins/deployment/AbstractKernelDeployer.java
      ===================================================================
      --- kernel/src/main/java/org/jboss/kernel/plugins/deployment/AbstractKernelDeployer.java
      +++ kernel/src/main/java/org/jboss/kernel/plugins/deployment/AbstractKernelDeployer.java
      @@ -217,22 +217,21 @@
       buffer.append("Incompletely deployed:\n");
       if (errors.size() != 0)
       {
      - buffer.append("\n*** DEPLOYMENTS IN ERROR: Name -> Error\n");
      + buffer.append("\nDEPLOYMENTS IN ERROR:\n");
       for (ControllerContext ctx : errors)
       {
      - buffer.append(ctx.getName()).append(" -> ").append(ctx.getError().toString()).append('\n');
      + buffer.append(String.format(" Deployment \"%s\" is in error due to: %s\n", ctx.getName(), ctx.getError()));
       }
       }
       if (incomplete.size() != 0)
       {
      - buffer.append("\n*** DEPLOYMENTS MISSING DEPENDENCIES: Name -> Dependency{Required State:Actual State}\n");
      + buffer.append("\nDEPLOYMENTS MISSING DEPENDENCIES:\n");
       for (ControllerContext ctx : incomplete)
       {
       Object name = ctx.getName();
      - buffer.append(name).append(" -> ");
      + buffer.append(String.format(" Deployment \"%s\" is missing the following dependencies:\n", name));
       DependencyInfo dependsInfo = ctx.getDependencyInfo();
       Set<DependencyItem> depends = dependsInfo.getIDependOn(null);
      - boolean first = true;
       for (DependencyItem item : depends)
       {
       ControllerState dependentState = item.getDependentState();
      @@ -262,29 +261,18 @@
      
       if (print)
       {
      - if (first)
      - first = false;
      - else
      - buffer.append(", ");
      -
      - buffer.append(iDependOn).append('{').append(dependentState.getStateString());
      - buffer.append(':');
      - if (iDependOn == null)
      - {
      - buffer.append("** UNRESOLVED " + item.toHumanReadableString() + " **");
      - }
      - else
      - {
      - if (other == null)
      - buffer.append("** NOT FOUND **");
      - else
      - buffer.append(otherState.getStateString());
      - }
      - buffer.append('}');
      + buffer.append(String.format(" Dependency \"%s\" (should be in state \"%s\", but is actually %s)\n",
      + iDependOn,
      + dependentState.getStateString(),
      + iDependOn == null ?
      + String.format("unresolved (%s)",
      + item.toHumanReadableString()) :
      + other == null ?
      + "not found" :
      + String.format("in state \"%s\"", otherState.getStateString())));
       }
       }
       }
      - buffer.append('\n');
       }
       }
       throw new IllegalStateException(buffer.toString());
      


        • 1. Re: Those terrible error messages
          Tim Fox Master

           

          "david.lloyd@jboss.com" wrote:
          One of the most off-putting things about JBossMC is the confusing, non-human error messages it produces when a deployment fails.


          +1. I was thinking exactly the same thing just the other day while trying to debug while JBM 2.0 wasn't loading in the MC, and struggling with the alien language.

          Anything to humanise it would be winner imho.

          • 2. Re: Those terrible error messages
            Ales Justin Master

             

            "david.lloyd@jboss.com" wrote:
            Should I commit it?

            Sure, go ahead.
            But this is not the only place this is done.
            Search Deployers code for similar error msg aka 'alien language'. ;-)

            You must fix them both or none. :-)

            • 3. Re: Those terrible error messages
              Ales Justin Master

               

              "timfox" wrote:

              I was thinking exactly the same thing just the other day while trying to debug while JBM 2.0 wasn't loading in the MC, and struggling with the alien language.

              Don't blame MC if JBM doesn't work. :-)

              • 4. Re: Those terrible error messages
                Trustin Lee Apprentice

                An off topic - I'd suggest you to replace '\n' with something more platform independent.

                For instance, Formatter got '%n'. I think we can't use System.getProperty() though due to the security issue as you already know. Perhaps there was something in JBoss Common? ;-)

                • 5. Re: Those terrible error messages
                  David Lloyd Master

                   

                  "alesj" wrote:
                  Sure, go ahead.
                  But this is not the only place this is done.
                  Search Deployers code for similar error msg aka 'alien language'. ;-)

                  You must fix them both or none. :-)


                  Ales, Ales, Ales. :-)

                  OK, I found some stuff in jboss-deployers-client-spi which I imagine is it. Here's that patch:

                  Index: deployers-client-spi/src/main/java/org/jboss/deployers/client/spi/IncompleteDeployments.java
                  ===================================================================
                  --- deployers-client-spi/src/main/java/org/jboss/deployers/client/spi/IncompleteDeployments.java
                  +++ deployers-client-spi/src/main/java/org/jboss/deployers/client/spi/IncompleteDeployments.java
                  @@ -30,6 +30,7 @@
                   import java.util.Set;
                   import java.util.TreeMap;
                   import java.util.TreeSet;
                  +import java.util.Iterator;
                  
                   /**
                   * IncompleteDeployments.
                  @@ -261,19 +262,18 @@
                   // Display all the missing dependencies
                   if (contextsMissingDependencies.isEmpty() == false)
                   {
                  - buffer.append("\n*** CONTEXTS MISSING DEPENDENCIES: Name -> Dependency{Required State:Actual State}\n\n");
                  + buffer.append("\nDEPLOYMENTS MISSING DEPENDENCIES:\n");
                   for (Map.Entry<String, Set<MissingDependency>> entry : contextsMissingDependencies.entrySet())
                   {
                   String name = entry.getKey();
                  - buffer.append(name).append("\n");
                  + buffer.append(String.format(" Deployment \"%s\" is missing the following dependencies:\n", name));
                   for (MissingDependency dependency : entry.getValue())
                   {
                  - buffer.append(" -> ").append(dependency.getDependency());
                  - buffer.append('{').append(dependency.getRequiredState());
                  - buffer.append(':').append(dependency.getActualState()).append("}");
                  - buffer.append("\n");
                  + buffer.append(String.format(" Dependency \"%s\" (should be in state \"%s\", but is actually in state \"%s\")\n",
                  + dependency.getDependency(),
                  + dependency.getRequiredState(),
                  + dependency.getActualState()));
                   }
                  - buffer.append('\n');
                  
                   // It is not a root cause if it has missing dependencies
                   rootCauses.remove(name);
                  @@ -286,22 +286,19 @@
                  
                   if (rootCauses.isEmpty() == false)
                   {
                  - buffer.append("\n*** CONTEXTS IN ERROR: Name -> Error\n\n");
                  + buffer.append("\nDEPLOYMENTS IN ERROR:\n");
                   for (String key : rootCauses.keySet())
                   {
                  - buffer.append(key).append(" -> ");
                  + buffer.append(String.format(" Deployment \"%s\" is in error due to the following reason(s): ", key));
                   Set<String> values = rootCauses.get(key);
                  - boolean first = true;
                  - for (String value : values)
                  - {
                  - if (first == false)
                  - buffer.append(" | ");
                  - else
                  - first = false;
                  -
                  - buffer.append(value);
                  - }
                  - buffer.append("\n\n");
                  + Iterator<String> it = values.iterator();
                  + while (it.hasNext())
                  + {
                  + buffer.append(it.next());
                  + if (it.hasNext())
                  + buffer.append(", ");
                  + }
                  + buffer.append("\n");
                   }
                   }
                   contextsInErrorInfo = buffer.toString();
                  


                  • 6. Re: Those terrible error messages
                    David Lloyd Master

                     

                    "trustin" wrote:
                    An off topic - I'd suggest you to replace '\n' with something more platform independent.

                    For instance, Formatter got '%n'. I think we can't use System.getProperty() though due to the security issue as you already know. Perhaps there was something in JBoss Common? ;-)


                    I believe that the logging framework should be taking care of this. Anyway we use '\n' very, very pervasively and I'm not going to try to fix every usage of it. :-)

                    • 7. Re: Those terrible error messages
                      David Lloyd Master

                      In any case, let me know if I shouldn't commit this, otherwise I will... today. :-)

                      • 8. Re: Those terrible error messages
                        Ales Justin Master

                         

                        "david.lloyd@jboss.com" wrote:
                        In any case, let me know if I shouldn't commit this, otherwise I will... today. :-)

                        DML, DML, DML ... commit them both. :-)

                        • 9. Re: Those terrible error messages
                          David Lloyd Master

                          Committed.

                          • 10. Re: Those terrible error messages
                            David Lloyd Master

                            Doing some testing in JBAS trunk, I triggered the new error message:

                            Failed to load profile: Summary of incomplete deployments (SEE PREVIOUS ERRORS FOR DETAILS):
                            
                            DEPLOYMENTS MISSING DEPENDENCIES:
                             Deployment "TestTCPServer" is missing the following dependencies:
                             Dependency "XnioProvider" (should be in state "Instantiated", but is actually in state "Described")
                             Deployment "XnioProvider" is missing the following dependencies:
                             Dependency "XnioSelectorThreadFactory" (should be in state "Instantiated", but is actually in state "Described")
                             Deployment "XnioSelectorThreadFactory" is missing the following dependencies:
                             Dependency "XnioSelectorThreadGroup" (should be in state "Instantiated", but is actually in state "Described")
                             Dependency "XnioThreadInterruptHandler" (should be in state "Instantiated", but is actually in state "** NOT FOUND Depends on 'XnioThreadInterruptHandler' **")
                             Deployment "XnioSelectorThreadGroup" is missing the following dependencies:
                             Dependency "xnio" (should be in state "Instantiated", but is actually in state "** NOT FOUND Depends on 'xnio' **")
                            
                            DEPLOYMENTS IN ERROR:
                             Deployment "xnio" is in error due to the following reason(s): ** NOT FOUND Depends on 'xnio' **
                             Deployment "XnioThreadInterruptHandler" is in error due to the following reason(s): ** NOT FOUND Depends on 'XnioThreadInterruptHandler' **
                            


                            It looks pretty nice I think. One thing that doesn't quite make sense though is the "** NOT FOUND" part... it says "Depends on XXX" but it's listing its own bean there. If I get a minute, I'll try to find where this is coming from and turn it into a nicer "not found" message...