This content has been marked as final.
Show 3 replies
-
1. Re: Bug in Microcontainer - Errors from DependencyInfo/Item
adrian.brock Jul 10, 2008 5:40 AM (in response to adrian.brock) -
2. Re: Bug in Microcontainer - Errors from DependencyInfo/Item
alesj Jul 11, 2008 9:39 AM (in response to adrian.brock)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 methodDependencyInfo 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 uninstallDependencyInfo 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 Jul 14, 2008 7:36 AM (in response to adrian.brock)"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.