Author: davidb
Date: Thu Jul 30 10:38:25 2015
New Revision: 1693406
URL: http://svn.apache.org/r1693406
Log:
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).
Modified:
felix/trunk/framework/src/main/java/org/apache/felix/framework/ServiceRegistry.java
felix/trunk/framework/src/test/java/org/apache/felix/framework/ServiceRegistryTest.java
Modified:
felix/trunk/framework/src/main/java/org/apache/felix/framework/ServiceRegistry.java
URL:
http://svn.apache.org/viewvc/felix/trunk/framework/src/main/java/org/apache/felix/framework/ServiceRegistry.java?rev=1693406&r1=1693405&r2=1693406&view=diff
==============================================================================
---
felix/trunk/framework/src/main/java/org/apache/felix/framework/ServiceRegistry.java
(original)
+++
felix/trunk/framework/src/main/java/org/apache/felix/framework/ServiceRegistry.java
Thu Jul 30 10:38:25 2015
@@ -317,17 +317,17 @@ public class ServiceRegistry
// 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 @@ public class ServiceRegistry
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 @@ public class ServiceRegistry
.getRegistration().ungetService(bundle, svc);
}
-
}
}
+
+ return count >= 0;
}
finally
{
@@ -455,7 +471,7 @@ public class ServiceRegistry
// 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 @@ public class ServiceRegistry
{
reg.unmarkCurrentThread();
}
-
- return true;
}
@@ -492,6 +506,9 @@ public class ServiceRegistry
// 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))
{
Modified:
felix/trunk/framework/src/test/java/org/apache/felix/framework/ServiceRegistryTest.java
URL:
http://svn.apache.org/viewvc/felix/trunk/framework/src/test/java/org/apache/felix/framework/ServiceRegistryTest.java?rev=1693406&r1=1693405&r2=1693406&view=diff
==============================================================================
---
felix/trunk/framework/src/test/java/org/apache/felix/framework/ServiceRegistryTest.java
(original)
+++
felix/trunk/framework/src/test/java/org/apache/felix/framework/ServiceRegistryTest.java
Thu Jul 30 10:38:25 2015
@@ -31,6 +31,8 @@ import java.util.concurrent.ConcurrentMa
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.
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 @@ public class ServiceRegistryTest extends
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 @@ public class ServiceRegistryTest extends
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 @@ public class ServiceRegistryTest extends
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 @@ public class ServiceRegistryTest extends
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 @@ public class ServiceRegistryTest extends
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
{
