FELIX-4977 Fix for concurrency issue with factory services
This commit fixes the issue in nearly all cases. Very occasionally the testGetUngetServiceFactory() test still reports 1 violation (down from the hundreds we were getting).
git-svn-id: https://svn.apache.org/repos/asf/felix/trunk@1693406 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/framework/src/main/java/org/apache/felix/framework/ServiceRegistry.java b/framework/src/main/java/org/apache/felix/framework/ServiceRegistry.java
index eefdddf..7f1132a 100644
--- a/framework/src/main/java/org/apache/felix/framework/ServiceRegistry.java
+++ b/framework/src/main/java/org/apache/felix/framework/ServiceRegistry.java
@@ -317,17 +317,17 @@
// Increment the usage count and grab the already retrieved
// service object, if one exists.
- checkCountOverflow(usage.m_count.incrementAndGet());
-
+ incrementToPositiveValue(usage.m_count);
svcObj = usage.getService();
+
if ( isServiceObjects )
{
- checkCountOverflow(usage.m_serviceObjectsCount.incrementAndGet());
+ incrementToPositiveValue(usage.m_serviceObjectsCount);
}
// If we have a usage count, but no service object, then we haven't
// cached the service object yet, so we need to create one.
- if ((usage != null) && (svcObj == null))
+ if (usage != null)
{
ServiceHolder holder = null;
@@ -379,6 +379,21 @@
return (S) svcObj;
}
+ // Increment the Atomic Long by 1, and ensure the result is at least 1.
+ private void incrementToPositiveValue(AtomicLong al)
+ {
+ boolean success = false;
+
+ while (!success)
+ {
+ long oldVal = al.get();
+ long newVal = Math.max(oldVal + 1L, 1L);
+ checkCountOverflow(newVal);
+
+ success = al.compareAndSet(oldVal, newVal);
+ }
+ }
+
private void checkCountOverflow(long c)
{
if (c == Long.MAX_VALUE)
@@ -442,9 +457,10 @@
.getRegistration().ungetService(bundle, svc);
}
-
}
}
+
+ return count >= 0;
}
finally
{
@@ -455,7 +471,7 @@
// If the registration is invalid or the usage count has reached
// zero, then flush it.
- if (count <= 0 || !reg.isValid())
+ if (!reg.isValid())
{
flushUsageCount(bundle, ref, usage);
}
@@ -465,8 +481,6 @@
{
reg.unmarkCurrentThread();
}
-
- return true;
}
@@ -492,6 +506,9 @@
// service cache.
for (int i = 0; i < usages.length; i++)
{
+ if (usages[i].m_svcHolderRef.get() == null)
+ continue;
+
// Keep ungetting until all usage count is zero.
while (ungetService(bundle, usages[i].m_ref, usages[i].m_prototype ? usages[i].getService() : null))
{
diff --git a/framework/src/test/java/org/apache/felix/framework/ServiceRegistryTest.java b/framework/src/test/java/org/apache/felix/framework/ServiceRegistryTest.java
index cdc1cb5..143ff58 100644
--- a/framework/src/test/java/org/apache/felix/framework/ServiceRegistryTest.java
+++ b/framework/src/test/java/org/apache/felix/framework/ServiceRegistryTest.java
@@ -31,6 +31,8 @@
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.atomic.AtomicBoolean;
+import junit.framework.TestCase;
+
import org.apache.felix.framework.ServiceRegistrationImpl.ServiceReferenceImpl;
import org.apache.felix.framework.ServiceRegistry.ServiceHolder;
import org.apache.felix.framework.ServiceRegistry.UsageCount;
@@ -51,8 +53,6 @@
import org.osgi.framework.hooks.service.FindHook;
import org.osgi.framework.hooks.service.ListenerHook;
-import junit.framework.TestCase;
-
public class ServiceRegistryTest extends TestCase
{
public void testRegisterEventHookService()
@@ -519,9 +519,8 @@
inUseMap.put(b, new UsageCount[] {uc});
- assertTrue(sr.ungetService(b, ref, null));
+ assertFalse(sr.ungetService(b, ref, null));
assertNull(uc.m_svcHolderRef.get());
- assertNull(inUseMap.get(b));
}
@SuppressWarnings("unchecked")
@@ -551,7 +550,6 @@
assertTrue(sr.ungetService(b, ref, null));
assertNull(uc.m_svcHolderRef.get());
- assertNull(inUseMap.get(b));
Mockito.verify(reg, Mockito.times(1)).
ungetService(Mockito.isA(Bundle.class), Mockito.eq(svc));
@@ -609,7 +607,6 @@
assertTrue(sr.ungetService(b, ref, null));
assertNull(uc.m_svcHolderRef.get());
- assertNull(inUseMap.get(b));
Mockito.verify(reg, Mockito.never()).
ungetService(Mockito.isA(Bundle.class), Mockito.any());
@@ -651,7 +648,6 @@
assertEquals("Test!", re.getMessage());
}
assertNull(uc.m_svcHolderRef.get());
- assertNull(inUseMap.get(b));
Mockito.verify(reg, Mockito.times(1)).ungetService(b, svc);
}
@@ -1150,6 +1146,27 @@
assertTrue("" + exceptions.size(), exceptions.isEmpty());
}
+ public void testUsageCountCleanup() throws Exception
+ {
+ ServiceRegistry sr = new ServiceRegistry(null, null);
+ Bundle regBundle = Mockito.mock(Bundle.class);
+
+ ServiceRegistration<?> reg = sr.registerService(
+ regBundle, new String [] {String.class.getName()}, "hi", null);
+
+ final Bundle clientBundle = Mockito.mock(Bundle.class);
+ Mockito.when(clientBundle.getBundleId()).thenReturn(327L);
+
+ assertEquals("hi", sr.getService(clientBundle, reg.getReference(), false));
+ sr.ungetService(clientBundle, reg.getReference(), null);
+
+ ConcurrentMap<Bundle, UsageCount[]> inUseMap =
+ (ConcurrentMap<Bundle, UsageCount[]>) getPrivateField(sr, "m_inUseMap");
+
+ sr.unregisterService(regBundle, reg);
+ assertEquals(0, inUseMap.size());
+ }
+
private Object getPrivateField(Object obj, String fieldName) throws NoSuchFieldException,
IllegalAccessException
{