3 Replies Latest reply on Jul 14, 2008 7:36 AM by adrian.brock

    Bug in Microcontainer - Errors from DependencyInfo/Item

      I've found a bug in the MC where if a DependencyItem.resolve()
      throws a RuntimeException that error doesn't get handled.

      It is propogated out to whoever did the install()/change()
      and further resolution stops.

      I don't think it leaves anything in an inconsistent state, but it does
      mean that everytime tries to install()/change() something
      (regardless of whether it is related to the context in error)
      they will get that error back.

      For the case I found, the fix is (which I will commit)

      Index: dependency/src/main/org/jboss/dependency/plugins/AbstractController.java
      ===================================================================
      --- dependency/src/main/org/jboss/dependency/plugins/AbstractController.java (revision 75590)
      +++ dependency/src/main/org/jboss/dependency/plugins/AbstractController.java (working copy)
      @@ -1017,8 +1017,18 @@
       if (advance(ctx))
       {
       DependencyInfo dependencies = ctx.getDependencyInfo();
      - if (dependencies.resolveDependencies(this, state))
      - result.add(ctx);
      + try
      + {
      + if (dependencies.resolveDependencies(this, state))
      + result.add(ctx);
      + }
      + catch (Throwable error)
      + {
      + log.error("Error resolving dependencies for " + state.getStateString() + ": " + ctx.toShortString(), error);
      + uninstallContext(ctx, ControllerState.NOT_INSTALLED, trace);
      + errorContexts.put(ctx.getName(), ctx);
      + ctx.setError(error);
      + }
       }
       }
       }
      


      Which treats the error from the DependencyInfo as a normal installation
      failure and moves that context to the error state.
      It then continues with other contexts.

      But this needs validating for all "callouts" to DependencyInfo
      and tests written for a "poison" DependencyInfo/Item implementation
      to make sure it doesn't cause the repeated error problem I described above.

        • 1. Re: Bug in Microcontainer - Errors from DependencyInfo/Item
          • 2. Re: Bug in Microcontainer - Errors from DependencyInfo/Item
            alesj

            I've added a test - BadDependencyInfoTestCase - that checks every method of DependencyInfo/Item, throwing an exception after a number of invocations.

            There is a few issues I found in AbstractController.

            1) trivial one - suppressed
            Dependency info in install method

             DependencyInfo dependencies = context.getDependencyInfo();
             if (trace)
             {
             String dependsOn = "[]";
             if (dependencies != null)
             {
             try
             {
             Set<DependencyItem> set = dependencies.getIDependOn(null);
             if (set != null)
             dependsOn = set.toString();
             }
             catch (Throwable t)
             {
             log.warn("Exception getting dependencies: " + t);
             dependsOn = null;
             }
             }
             if (dependsOn != null)
             log.trace("Dependencies for " + name + ": " + dependsOn);
            


            2) moving to error - no uninstall since we're in uninstall
             DependencyInfo dependencies = context.getDependencyInfo();
             if (dependencies != null)
             {
             try
             {
             Set<DependencyItem> dependsOnMe = dependencies.getDependsOnMe(null);
             if (dependsOnMe.isEmpty() == false)
             {
             for (DependencyItem item : dependsOnMe)
             {
             if (item.isResolved())
             {
             ControllerState dependentState = item.getDependentState();
             if (dependentState == null || dependentState.equals(fromState))
             {
             if (item.unresolved(this))
             {
             ControllerContext dependent = getContext(item.getName(), null);
             if (dependent != null)
             {
             ControllerState whenRequired = item.getWhenRequired();
             if (whenRequired == null)
             whenRequired = ControllerState.NOT_INSTALLED;
             if (isBeforeState(dependent.getState(), whenRequired) == false)
             uninstallContext(dependent, whenRequired, trace);
             }
             }
             }
             }
             }
             }
             }
             catch (Throwable error)
             {
             log.error("Error resolving dependencies for " + fromState.getStateString() + ": " + context.toShortString(), error);
             // Not much else we can do - no uninstall, since we are at uninstall ;-)
             errorContexts.put(context.getName(), context);
             context.setError(error);
             }
             }
            


            3) Currently not testing this:
             for (Method method : methods)
             {
             // Should we suppress this?
             if ("getLifecycleCallbacks".equals(method.getName()))
             continue;
            

            If handleInstallLifecycleCallbacks throws exception ((in initial incrementState)) on DependencyInfo::getLifecycleCallbacks(), it gets caught, but re-thrown:
             boolean ok = incrementState(context, trace);
             if (ok)
             {
             try
             {
             registerControllerContext(context);
             }
             catch (Throwable t)
             {
             // This is probably unreachable? But let's be paranoid
             ok = false;
             throw t;
             }
             }
             if (ok)
             {
             resolveContexts(trace);
             }
             else
             {
             errorContexts.remove(context);
             throw context.getError();
             }
            

            Should we add context-2-error-handling code to this method?

            • 3. Re: Bug in Microcontainer - Errors from DependencyInfo/Item

               

              "alesj" wrote:

              3) Currently not testing this:
               for (Method method : methods)
               {
               // Should we suppress this?
               if ("getLifecycleCallbacks".equals(method.getName()))
               continue;
              

              If handleInstallLifecycleCallbacks throws exception ((in initial incrementState)) on DependencyInfo::getLifecycleCallbacks(), it gets caught, but re-thrown:
               boolean ok = incrementState(context, trace);
               if (ok)
               {
               try
               {
               registerControllerContext(context);
               }
               catch (Throwable t)
               {
               // This is probably unreachable? But let's be paranoid
               ok = false;
               throw t;
               }
               }
               if (ok)
               {
               resolveContexts(trace);
               }
               else
               {
               errorContexts.remove(context);
               throw context.getError();
               }
              

              Should we add context-2-error-handling code to this method?


              You need to write that test, but it looks to me that currently the case
              is already handled since the handleInstallLifecycleCallbacks is in a try/catch block
              that moves the context to the error state and then the code you show
              removes it altogether and rethrows the error.