3 Replies Latest reply on Jul 14, 2008 7:36 AM by Adrian Brock

    Bug in Microcontainer - Errors from DependencyInfo/Item

    Adrian Brock Master

      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.

        • 2. Re: Bug in Microcontainer - Errors from DependencyInfo/Item
          Ales Justin Master

          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
            Adrian Brock Master

             

            "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.