Bug 563987 - Handle service rankings

The implementation did not track service ranking.  If rankings changed
such that the cached service is no longer the highest ranked the
implementation would keep returning the lower ranked instance.

Change-Id: I086ed3e9b7f9ca2e2005cb304b3c8e8348c9be1a
Signed-off-by: Thomas Watson <tjwatson@us.ibm.com>

2 files changed, 101 insertions, 12 deletions

diff --git a/bundles/org.eclipse.equinox.common.tests/src/org/eclipse/equinox/common/tests/ServiceCallerTest.java b/bundles/org.eclipse.equinox.common.tests/src/org/eclipse/equinox/common/tests/ServiceCallerTest.java
index b77e5bfbd..1be98fb45 100644
--- a/bundles/org.eclipse.equinox.common.tests/src/org/eclipse/equinox/common/tests/ServiceCallerTest.java
+++ b/bundles/org.eclipse.equinox.common.tests/src/org/eclipse/equinox/common/tests/ServiceCallerTest.java
@@ -13,7 +13,9 @@
  *******************************************************************************/
 package org.eclipse.equinox.common.tests;
 
+import static java.util.Collections.singletonMap;
 import static org.junit.Assert.assertNotEquals;
+import static org.osgi.framework.FrameworkUtil.asDictionary;
 
 import java.io.IOException;
 import java.util.Arrays;
@@ -29,6 +31,7 @@ import org.eclipse.core.tests.harness.CoreTest;
 import org.osgi.framework.Bundle;
 import org.osgi.framework.BundleContext;
 import org.osgi.framework.BundleException;
+import org.osgi.framework.Constants;
 import org.osgi.framework.FrameworkUtil;
 import org.osgi.framework.InvalidSyntaxException;
 import org.osgi.framework.ServiceFactory;
@@ -207,6 +210,64 @@ public class ServiceCallerTest extends CoreTest {
 		}
 	}
 
+	public void testRank() {
+		Bundle bundle = FrameworkUtil.getBundle(ServiceCallerTest.class);
+		assertNotNull("Test only works under an OSGi runtime", bundle);
+		BundleContext context = bundle.getBundleContext();
+		ServiceExample s1 = new ServiceExample();
+		ServiceExample s2 = new ServiceExample();
+		ServiceExample s3 = new ServiceExample();
+		ServiceRegistration<IServiceExample> reg1 = null;
+		ServiceRegistration<IServiceExample> reg2 = null;
+		ServiceRegistration<IServiceExample> reg3 = null;
+		try {
+			reg1 = context.registerService(IServiceExample.class, s1,
+					asDictionary(singletonMap(Constants.SERVICE_RANKING, 1)));
+			reg2 = context.registerService(IServiceExample.class, s2,
+					asDictionary(singletonMap(Constants.SERVICE_RANKING, 2)));
+			reg3 = context.registerService(IServiceExample.class, s3,
+					asDictionary(singletonMap(Constants.SERVICE_RANKING, 3)));
+			ServiceCaller<IServiceExample> caller = new ServiceCaller<>(getClass(), IServiceExample.class);
+
+			assertHighestRankedCalled(caller, s3, s2, s1, reg3, reg2, reg1);
+
+			reg1.setProperties(asDictionary(singletonMap(Constants.SERVICE_RANKING, 10)));
+			assertHighestRankedCalled(caller, s1, s2, s3, reg1, reg2, reg3);
+
+			reg2.setProperties(asDictionary(singletonMap(Constants.SERVICE_RANKING, 10)));
+			reg1.setProperties(asDictionary(singletonMap(Constants.SERVICE_RANKING, 11)));
+			assertHighestRankedCalled(caller, s1, s2, s3, reg1, reg2, reg3);
+
+			reg1.setProperties(asDictionary(singletonMap(Constants.SERVICE_RANKING, 1)));
+			assertHighestRankedCalled(caller, s2, s1, s3, reg2, reg1, reg3);
+		} finally {
+			if (reg1 != null) {
+				reg1.unregister();
+			}
+			if (reg2 != null) {
+				reg2.unregister();
+			}
+			if (reg3 != null) {
+				reg3.unregister();
+			}
+		}
+	}
+
+	private void assertHighestRankedCalled(ServiceCaller<IServiceExample> caller, ServiceExample highest,
+			ServiceExample lower1, ServiceExample lower2, ServiceRegistration<IServiceExample> highestReg,
+			ServiceRegistration<IServiceExample> lower1Reg, ServiceRegistration<IServiceExample> lower2Reg) {
+		assertTrue("Did not call service.", caller.call(IServiceExample::call));
+		assertTrue("Highest Ranked not called.", highest.called);
+		assertFalse("Lower1 called.", lower1.called);
+		assertFalse("Lower2 called.", lower2.called);
+		assertNotNull("No users of highest.", highestReg.getReference().getUsingBundles());
+		assertNull("Users of lower ranked.", lower1Reg.getReference().getUsingBundles());
+		assertNull("Users of lower ranked.", lower2Reg.getReference().getUsingBundles());
+		highest.called = false;
+		lower1.called = false;
+		lower2.called = false;
+	}
+
 	interface IServiceExample {
 		void call();
 	}
diff --git a/bundles/org.eclipse.equinox.common/src/org/eclipse/core/runtime/ServiceCaller.java b/bundles/org.eclipse.equinox.common/src/org/eclipse/core/runtime/ServiceCaller.java
index 37f7570b1..bd472f563 100644
--- a/bundles/org.eclipse.equinox.common/src/org/eclipse/core/runtime/ServiceCaller.java
+++ b/bundles/org.eclipse.equinox.common/src/org/eclipse/core/runtime/ServiceCaller.java
@@ -103,15 +103,25 @@ public class ServiceCaller<Service> {
 		return new ServiceCaller<>(caller, serviceType).getCallUnget(consumer);
 	}
 
+	static int getRank(ServiceReference<?> ref) {
+		Object rank = ref.getProperty(Constants.SERVICE_RANKING);
+		if (rank instanceof Integer) {
+			return ((Integer) rank).intValue();
+		}
+		return 0;
+	}
+
 	class ReferenceAndService implements SynchronousBundleListener, ServiceListener {
 		final BundleContext context;
 		final ServiceReference<Service> ref;
 		final Service instance;
+		final int rank;
 
 		public ReferenceAndService(final BundleContext context, ServiceReference<Service> ref, Service instance) {
 			this.context = context;
 			this.ref = ref;
 			this.instance = instance;
+			this.rank = getRank(ref);
 		}
 
 		void unget() {
@@ -126,17 +136,28 @@ public class ServiceCaller<Service> {
 		@Override
 		public void bundleChanged(BundleEvent e) {
 			if (bundle.equals(e.getBundle()) && e.getType() == BundleEvent.STOPPING) {
-				untrack();
+				unget();
 			}
 		}
 
 		@Override
 		public void serviceChanged(ServiceEvent e) {
-			if (e.getType() == ServiceEvent.UNREGISTERING) {
-				untrack();
-			}
-			if (filter != null && e.getType() == ServiceEvent.MODIFIED_ENDMATCH) {
-				untrack();
+			if (e.getServiceReference().equals(ref)) {
+				if (e.getType() == ServiceEvent.UNREGISTERING) {
+					unget();
+				} else if (filter != null && e.getType() == ServiceEvent.MODIFIED_ENDMATCH) {
+					unget();
+				} else if (e.getType() == ServiceEvent.MODIFIED) {
+					if (getRank(ref) != rank) {
+						// rank changed; untrack to force a new ReferenceAndService with new rank
+						unget();
+					}
+				}
+			} else if (e.getType() == ServiceEvent.MODIFIED) {
+				if (getRank(e.getServiceReference()) > rank) {
+					// Another service with higher rank is available
+					unget();
+				}
 			}
 		}
 
@@ -144,18 +165,19 @@ public class ServiceCaller<Service> {
 		Optional<ReferenceAndService> track() {
 			try {
 				ServiceCaller.this.service = this;
-				// Filter specific to this service reference ID
 				context.addServiceListener(this, "(&" //$NON-NLS-1$
 						+ "(objectClass=" + serviceType.getName() + ")" // //$NON-NLS-1$ //$NON-NLS-2$
-						+ "(service.id=" + ref.getProperty(Constants.SERVICE_ID) + ")" // //$NON-NLS-1$ //$NON-NLS-2$
 						+ (filter == null ? "" : filter) // //$NON-NLS-1$
 						+ ")"); //$NON-NLS-1$
 				context.addBundleListener(this);
-				if (ref.getBundle() == null || context.getBundle() == null && ServiceCaller.this.service == this) {
+				if ((ref.getBundle() == null || context.getBundle() == null) && ServiceCaller.this.service == this) {
 					// service should have been untracked but we may have missed the event
 					// before we could added the listeners
-					untrack();
-					return Optional.empty();
+					unget();
+				}
+				if (getRank(ref) != rank) {
+					// ranking has changed; unget to force reget in case the ranking is not the highest
+					unget();
 				}
 			} catch (InvalidSyntaxException e) {
 				// really should never happen with our own filter above.
@@ -164,8 +186,12 @@ public class ServiceCaller<Service> {
 			} catch (IllegalStateException e) {
 				// bundle was stopped before we could get listeners added/removed
 				ServiceCaller.this.service = null;
-				return Optional.empty();
 			}
+			// Note that we always return this ReferenceAndService
+			// even for cases where the instance was unget
+			// It is way complicated to try again and
+			// even if we did the returned value can become
+			// stale right after return.
 			return Optional.of(this);
 		}
 
@@ -247,7 +273,9 @@ public class ServiceCaller<Service> {
 	 * <ul>
 	 * <li>The {@link #unget()} method is called.</li>
 	 * <li>The service is unregistered.</li>
+	 * <li>The service properties change such that this {@code ServiceCaller} filter no longer matches.
 	 * <li>The caller bundle is stopped.</li>
+	 * <li>The service rankings have changed.</li>
 	 * </ul>
 	 * 
 	 * After one of these conditions occur subsequent calls to this method will try to acquire the