Bug 348805 - Calling ResolverHook.end in failure case

4 files changed, 132 insertions, 39 deletions

diff --git a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/ClassLoadingBundleTests.java b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/ClassLoadingBundleTests.java
index f961165d3..1c537882b 100644
--- a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/ClassLoadingBundleTests.java
+++ b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/ClassLoadingBundleTests.java
@@ -19,7 +19,9 @@ import junit.framework.TestSuite;
 import org.eclipse.osgi.service.resolver.*;
 import org.eclipse.osgi.tests.OSGiTestsActivator;
 import org.osgi.framework.*;
-import org.osgi.framework.wiring.BundleWiring;
+import org.osgi.framework.hooks.resolver.ResolverHook;
+import org.osgi.framework.hooks.resolver.ResolverHookFactory;
+import org.osgi.framework.wiring.*;
 import org.osgi.service.packageadmin.ExportedPackage;
 import org.osgi.service.packageadmin.PackageAdmin;
 import org.osgi.service.startlevel.StartLevel;
@@ -1615,6 +1617,86 @@ public class ClassLoadingBundleTests extends AbstractBundleTests {
 		}
 	}
 
+	public void testBug348805() {
+		final boolean[] endCalled = {false};
+		ResolverHookFactory error = new ResolverHookFactory() {
+			public ResolverHook begin(Collection triggers) {
+				return new ResolverHook() {
+					public void filterSingletonCollisions(BundleCapability singleton, Collection collisionCandidates) {
+						// Nothing
+					}
+
+					public void filterResolvable(Collection candidates) {
+						throw new RuntimeException("Error");
+					}
+
+					public void filterMatches(BundleRequirement requirement, Collection candidates) {
+						// Nothing
+					}
+
+					public void end() {
+						endCalled[0] = true;
+					}
+				};
+			}
+		};
+		ServiceRegistration reg = OSGiTestsActivator.getContext().registerService(ResolverHookFactory.class, error, null);
+		try {
+			Bundle test = installer.installBundle("test"); //$NON-NLS-1$
+			try {
+				test.start();
+				fail("Should not be able to start this bundle");
+			} catch (BundleException e) {
+				// expected
+				assertEquals("Wrong exception type.", BundleException.REJECTED_BY_HOOK, e.getType());
+			}
+		} catch (BundleException e) {
+			fail("Unexpected install fail", e);
+		} finally {
+			reg.unregister();
+		}
+		assertTrue("end is not called", endCalled[0]);
+	}
+
+	public void testBug348806() {
+		ResolverHookFactory error = new ResolverHookFactory() {
+			public ResolverHook begin(Collection triggers) {
+				return new ResolverHook() {
+					public void filterSingletonCollisions(BundleCapability singleton, Collection collisionCandidates) {
+						// Nothing
+					}
+
+					public void filterResolvable(Collection candidates) {
+						// Nothing
+					}
+
+					public void filterMatches(BundleRequirement requirement, Collection candidates) {
+						// Nothing
+					}
+
+					public void end() {
+						throw new RuntimeException("Error");
+					}
+				};
+			}
+		};
+		ServiceRegistration reg = OSGiTestsActivator.getContext().registerService(ResolverHookFactory.class, error, null);
+		try {
+			Bundle test = installer.installBundle("test"); //$NON-NLS-1$
+			try {
+				test.start();
+				fail("Should not be able to start this bundle");
+			} catch (BundleException e) {
+				// expected
+				assertEquals("Wrong exception type.", BundleException.REJECTED_BY_HOOK, e.getType());
+			}
+		} catch (BundleException e) {
+			fail("Unexpected install fail", e);
+		} finally {
+			reg.unregister();
+		}
+	}
+
 	private void doTestArrayTypeLoad(String name) {
 		try {
 			Class arrayType = OSGiTestsActivator.getContext().getBundle().loadClass(name);
diff --git a/bundles/org.eclipse.osgi/core/framework/org/eclipse/osgi/framework/internal/core/CoreResolverHookFactory.java b/bundles/org.eclipse.osgi/core/framework/org/eclipse/osgi/framework/internal/core/CoreResolverHookFactory.java
index 90a580112..3e5b3cffc 100644
--- a/bundles/org.eclipse.osgi/core/framework/org/eclipse/osgi/framework/internal/core/CoreResolverHookFactory.java
+++ b/bundles/org.eclipse.osgi/core/framework/org/eclipse/osgi/framework/internal/core/CoreResolverHookFactory.java
@@ -15,7 +15,8 @@ import org.eclipse.osgi.framework.debug.Debug;
 import org.eclipse.osgi.internal.serviceregistry.*;
 import org.eclipse.osgi.service.resolver.ResolverHookException;
 import org.eclipse.osgi.util.NLS;
-import org.osgi.framework.*;
+import org.osgi.framework.Bundle;
+import org.osgi.framework.InvalidSyntaxException;
 import org.osgi.framework.hooks.resolver.ResolverHook;
 import org.osgi.framework.hooks.resolver.ResolverHookFactory;
 import org.osgi.framework.wiring.*;
@@ -49,19 +50,14 @@ public class CoreResolverHookFactory implements ResolverHookFactory {
 		this.registry = registry;
 	}
 
-	void handleHookException(Throwable t, Object hook, String method, Bundle hookBundle, List<HookReference> hookRefs, boolean causeFailure) {
+	void handleHookException(Throwable t, Object hook, String method, Bundle hookBundle, List<HookReference> hookRefs) {
 		if (Debug.DEBUG_HOOKS) {
 			Debug.println(hook.getClass().getName() + "." + method + "() exception:"); //$NON-NLS-1$ //$NON-NLS-2$
 			if (t != null)
 				Debug.printStackTrace(t);
 		}
 		String message = NLS.bind(Msg.SERVICE_FACTORY_EXCEPTION, hook.getClass().getName(), method);
-		if (causeFailure) {
-			releaseHooks(hookRefs);
-			throw new ResolverHookException(message, t);
-		}
-		BundleException be = new BundleException(message, BundleException.REJECTED_BY_HOOK, t);
-		context.framework.publishFrameworkEvent(FrameworkEvent.ERROR, hookBundle, be);
+		throw new ResolverHookException(message, t);
 	}
 
 	private ServiceReferenceImpl<ResolverHookFactory>[] getHookReferences() {
@@ -91,7 +87,7 @@ public class CoreResolverHookFactory implements ResolverHookFactory {
 						if (hook != null)
 							hookRefs.add(new HookReference(hookRef, hook));
 					} catch (Throwable t) {
-						handleHookException(t, factory, "begin", hookRef.getBundle(), hookRefs, true); //$NON-NLS-1$
+						handleHookException(t, factory, "begin", hookRef.getBundle(), hookRefs); //$NON-NLS-1$
 					}
 				}
 			}
@@ -121,12 +117,12 @@ public class CoreResolverHookFactory implements ResolverHookFactory {
 			for (Iterator<HookReference> iHooks = hooks.iterator(); iHooks.hasNext();) {
 				HookReference hookRef = iHooks.next();
 				if (hookRef.reference.getBundle() == null) {
-					handleHookException(null, hookRef.hook, "filterResolvable", hookRef.reference.getBundle(), hooks, true); //$NON-NLS-1$
+					handleHookException(null, hookRef.hook, "filterResolvable", hookRef.reference.getBundle(), hooks); //$NON-NLS-1$
 				} else {
 					try {
 						hookRef.hook.filterResolvable(candidates);
 					} catch (Throwable t) {
-						handleHookException(t, hookRef.hook, "filterResolvable", hookRef.reference.getBundle(), hooks, true); //$NON-NLS-1$
+						handleHookException(t, hookRef.hook, "filterResolvable", hookRef.reference.getBundle(), hooks); //$NON-NLS-1$
 					}
 				}
 			}
@@ -142,12 +138,12 @@ public class CoreResolverHookFactory implements ResolverHookFactory {
 			for (Iterator<HookReference> iHooks = hooks.iterator(); iHooks.hasNext();) {
 				HookReference hookRef = iHooks.next();
 				if (hookRef.reference.getBundle() == null) {
-					handleHookException(null, hookRef.hook, "filterSingletonCollisions", hookRef.reference.getBundle(), hooks, true); //$NON-NLS-1$
+					handleHookException(null, hookRef.hook, "filterSingletonCollisions", hookRef.reference.getBundle(), hooks); //$NON-NLS-1$
 				} else {
 					try {
 						hookRef.hook.filterSingletonCollisions(singleton, collisionCandidates);
 					} catch (Throwable t) {
-						handleHookException(t, hookRef.hook, "filterSingletonCollisions", hookRef.reference.getBundle(), hooks, true); //$NON-NLS-1$
+						handleHookException(t, hookRef.hook, "filterSingletonCollisions", hookRef.reference.getBundle(), hooks); //$NON-NLS-1$
 					}
 				}
 			}
@@ -163,12 +159,12 @@ public class CoreResolverHookFactory implements ResolverHookFactory {
 			for (Iterator<HookReference> iHooks = hooks.iterator(); iHooks.hasNext();) {
 				HookReference hookRef = iHooks.next();
 				if (hookRef.reference.getBundle() == null) {
-					handleHookException(null, hookRef.hook, "filterMatches", hookRef.reference.getBundle(), hooks, true); //$NON-NLS-1$
+					handleHookException(null, hookRef.hook, "filterMatches", hookRef.reference.getBundle(), hooks); //$NON-NLS-1$
 				} else {
 					try {
 						hookRef.hook.filterMatches(requirement, candidates);
 					} catch (Throwable t) {
-						handleHookException(t, hookRef.hook, "filterMatches", hookRef.reference.getBundle(), hooks, true); //$NON-NLS-1$
+						handleHookException(t, hookRef.hook, "filterMatches", hookRef.reference.getBundle(), hooks); //$NON-NLS-1$
 					}
 				}
 			}
@@ -180,19 +176,22 @@ public class CoreResolverHookFactory implements ResolverHookFactory {
 			}
 			if (hooks.isEmpty())
 				return;
-			for (Iterator<HookReference> iHooks = hooks.iterator(); iHooks.hasNext();) {
-				HookReference hookRef = iHooks.next();
-				// We do not remove unregistered services here because we are going to remove of of them at the end
-				if (hookRef.reference.getBundle() != null) {
-					try {
-						hookRef.hook.end();
-					} catch (Throwable t) {
-						handleHookException(t, hookRef.hook, "end", hookRef.reference.getBundle(), hooks, false); //$NON-NLS-1$
-					}
+			try {
+				for (Iterator<HookReference> iHooks = hooks.iterator(); iHooks.hasNext();) {
+					HookReference hookRef = iHooks.next();
+					// We do not remove unregistered services here because we are going to remove of of them at the end
+					if (hookRef.reference.getBundle() != null) {
+						try {
+							hookRef.hook.end();
+						} catch (Throwable t) {
+							handleHookException(t, hookRef.hook, "end", hookRef.reference.getBundle(), hooks); //$NON-NLS-1$
+						}
 
+					}
 				}
+			} finally {
+				releaseHooks(hooks);
 			}
-			releaseHooks(hooks);
 		}
 	}
 }
diff --git a/bundles/org.eclipse.osgi/resolver/src/org/eclipse/osgi/internal/module/ResolverImpl.java b/bundles/org.eclipse.osgi/resolver/src/org/eclipse/osgi/internal/module/ResolverImpl.java
index 98745d0d2..defb5a28a 100644
--- a/bundles/org.eclipse.osgi/resolver/src/org/eclipse/osgi/internal/module/ResolverImpl.java
+++ b/bundles/org.eclipse.osgi/resolver/src/org/eclipse/osgi/internal/module/ResolverImpl.java
@@ -434,7 +434,21 @@ public class ResolverImpl implements Resolver {
 			ResolverBundle[] bundles = unresolvedBundles.toArray(new ResolverBundle[unresolvedBundles.size()]);
 
 			usesCalculationTimeout = false;
+
 			resolveBundles(bundles, platformProperties, hookDisabled);
+
+			@SuppressWarnings("unchecked")
+			Collection<ResolverBundle> optionalResolved = resolveOptional ? resolveOptionalConstraints(currentlyResolved) : Collections.EMPTY_LIST;
+			ResolverHook current = hook;
+			if (current != null) {
+				hook = null;
+				current.end();
+			}
+
+			for (ResolverBundle bundle : optionalResolved) {
+				state.resolveBundle(bundle.getBundleDescription(), false, null, null, null, null, null, null, null, null);
+				stateResolveBundle(bundle);
+			}
 			// reorder exports and bundles after resolving the bundles
 			resolverExports.reorder();
 			resolverBundles.reorder();
@@ -444,6 +458,8 @@ public class ResolverImpl implements Resolver {
 			if (DEBUG)
 				ResolverImpl.log("*** END RESOLUTION ***"); //$NON-NLS-1$
 		} finally {
+			if (hook != null)
+				hook.end(); // need to make sure end is always called
 			hook = null;
 		}
 	}
@@ -491,15 +507,18 @@ public class ResolverImpl implements Resolver {
 
 	}
 
-	private void resolveOptionalConstraints(ResolverBundle[] bundles) {
+	private Collection<ResolverBundle> resolveOptionalConstraints(ResolverBundle[] bundles) {
+		Collection<ResolverBundle> result = new ArrayList<ResolverBundle>();
 		for (int i = 0; i < bundles.length; i++) {
-			if (bundles[i] != null)
-				resolveOptionalConstraints(bundles[i]);
+			if (bundles[i] != null && resolveOptionalConstraints(bundles[i])) {
+				result.add(bundles[i]);
+			}
 		}
+		return result;
 	}
 
 	// TODO this does not do proper uses constraint verification.
-	private void resolveOptionalConstraints(ResolverBundle bundle) {
+	private boolean resolveOptionalConstraints(ResolverBundle bundle) {
 		BundleConstraint[] requires = bundle.getRequires();
 		List<ResolverBundle> cycle = new ArrayList<ResolverBundle>();
 		boolean resolvedOptional = false;
@@ -518,11 +537,7 @@ public class ResolverImpl implements Resolver {
 				if (imports[i].getSelectedSupplier() != null)
 					resolvedOptional = true;
 			}
-		if (resolvedOptional) {
-			state.resolveBundle(bundle.getBundleDescription(), false, null, null, null, null, null, null, null, null);
-			stateResolveConstraints(bundle);
-			stateResolveBundle(bundle);
-		}
+		return resolvedOptional;
 	}
 
 	private void getCurrentEEs(Dictionary<Object, Object>[] platformProperties) {
diff --git a/bundles/org.eclipse.osgi/resolver/src/org/eclipse/osgi/internal/resolver/StateImpl.java b/bundles/org.eclipse.osgi/resolver/src/org/eclipse/osgi/internal/resolver/StateImpl.java
index 231ec76a8..6f7b4d2e5 100644
--- a/bundles/org.eclipse.osgi/resolver/src/org/eclipse/osgi/internal/resolver/StateImpl.java
+++ b/bundles/org.eclipse.osgi/resolver/src/org/eclipse/osgi/internal/resolver/StateImpl.java
@@ -421,7 +421,6 @@ public abstract class StateImpl implements State {
 				throw new IllegalStateException("no resolver set"); //$NON-NLS-1$
 			if (resolving == true)
 				throw new IllegalStateException("An attempt to start a nested resolve process has been detected."); //$NON-NLS-1$
-			ResolverHook currentHook = null;
 			try {
 				resolving = true;
 				long start = 0;
@@ -474,7 +473,7 @@ public abstract class StateImpl implements State {
 				if (currentFactory != null) {
 					@SuppressWarnings("unchecked")
 					Collection<BundleRevision> triggerRevisions = Collections.unmodifiableCollection(triggers == null ? Collections.EMPTY_LIST : Arrays.asList((BundleRevision[]) triggers));
-					currentHook = begin(triggerRevisions);
+					begin(triggerRevisions);
 				}
 				ResolverHookException error = null;
 				try {
@@ -499,8 +498,6 @@ public abstract class StateImpl implements State {
 					updateTimeStamp();
 				return savedChanges;
 			} finally {
-				if (currentHook != null)
-					currentHook.end();
 				resolving = false;
 			}
 		}