You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@felix.apache.org by tj...@apache.org on 2020/09/10 15:43:16 UTC
[felix-dev] branch master updated: FELIX-6327 -
NoSuchElementException when services are removed
This is an automated email from the ASF dual-hosted git repository.
tjwatson pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/felix-dev.git
The following commit(s) were added to refs/heads/master by this push:
new 87f3aff FELIX-6327 - NoSuchElementException when services are removed
new c3f6eba Merge pull request #47 from tjwatson/FELIX-6327
87f3aff is described below
commit 87f3aff5aeb49ace8fa72245999829643fd4b480
Author: Thomas Watson <tj...@us.ibm.com>
AuthorDate: Thu Sep 10 10:10:22 2020 -0500
FELIX-6327 - NoSuchElementException when services are removed
The tryInvokeBind method needs more safeguards around when the reference
that just got bound is detected that it got removed.
The previous code always assumed there was at least one other viable
reference to bind to. That causes the NoSuchElementException when
trying to find a fallback reference. Fixing that exposed another issue
with the previous code where the state of the DependencyManager is not
properly restored in two aspects:
1) Even when unbind is not invoked during tryInvokeBind the
currentRefPair would get set to null when it must remain set to the
previous value in cases where the invoke bind fails. A check is needed
to be sure to only clear the currentRefPair if unbind was successfully
invoked.
2) After invoking bind/unbind there is a check to see if the reference
is still viable (not deleted). This is where the reported problem
occurs when there is no alternative reference available and a
NoSuchElementException is thrown. Avoiding the NoSuchElementException
and returning from tryInvokeBind would leave the DependencyManager in a
state where it thinks there is still a binding thread doing work when
that thread is really done. Additional checks are needed to ensure we
always restore the binding thread state of the DepenencyManager on exit
of tryInvokeBind method.
---
.../felix/scr/impl/manager/DependencyManager.java | 111 ++++++++++++++++-----
.../scr/integration/ServiceBindGreedyTest.java | 54 +++++++---
.../integration/components/SimpleComponent.java | 8 +-
.../integration/components/SimpleServiceImpl.java | 24 +++--
4 files changed, 144 insertions(+), 53 deletions(-)
diff --git a/scr/src/main/java/org/apache/felix/scr/impl/manager/DependencyManager.java b/scr/src/main/java/org/apache/felix/scr/impl/manager/DependencyManager.java
index 915faec..8d0b650 100644
--- a/scr/src/main/java/org/apache/felix/scr/impl/manager/DependencyManager.java
+++ b/scr/src/main/java/org/apache/felix/scr/impl/manager/DependencyManager.java
@@ -22,6 +22,7 @@ import java.security.Permission;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
+import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.SortedMap;
@@ -840,10 +841,7 @@ public class DependencyManager<S, T> implements ReferenceManager<S, T>
Object monitor = tracker == null ? null : tracker.tracked();
if (monitor != null)
{
- RefPair<S, T> next = refPair;
- while ((next = tryinvokeBind(tracker, monitor, next, trackingCount)) != null)
- {
- }
+ tryInvokeBind(tracker, monitor, refPair, trackingCount);
}
}
else if (isTrackerOpened() && cardinalityJustSatisfied(serviceCount))
@@ -863,7 +861,67 @@ public class DependencyManager<S, T> implements ReferenceManager<S, T>
}
}
- private RefPair<S, T> tryinvokeBind(
+ private void tryInvokeBind(
+ ServiceTracker<T, RefPair<S, T>, ExtendedServiceEvent> tracker,
+ Object monitor, RefPair<S, T> next, int trackingCount)
+ {
+ boolean checkQueue = false;
+ do
+ {
+ try
+ {
+ while ((next = tryInvokeBind0(tracker, monitor, next,
+ trackingCount)) != null)
+ {
+ }
+ }
+ finally
+ {
+ // be sure to clean up the bindingThread state
+ synchronized (monitor)
+ {
+ if (bindingThread != null
+ && bindingThread.equals(Thread.currentThread()))
+ {
+ // check that another thread didn't give us more work to do since
+ // the time we gave up the lock in tryInvokeBind0
+ if (queuedRefPairs.isEmpty())
+ {
+ // working thread is done, make sure state is cleared
+ bindingRefPair = null;
+ bindingThread = null;
+ }
+ else
+ {
+ next = getBestFromQueue();
+ // NOTE - getBestFromQueue cannot return null at this point
+ // because queuedRefPairs is not empty; always loop
+ // around to tryInvokeBind again in this case
+ checkQueue = true;
+ }
+ }
+ }
+ }
+ }
+ while (checkQueue);
+ }
+
+ private RefPair<S, T> getBestFromQueue()
+ {
+ RefPair<S, T> currentBest = null;
+ for (RefPair<S, T> betterCandidate : queuedRefPairs)
+ {
+ if (currentBest == null
+ || betterCandidate.getRef().compareTo(currentBest.getRef()) > 0)
+ {
+ currentBest = betterCandidate;
+ }
+ }
+ queuedRefPairs.clear();
+ return currentBest;
+ }
+
+ private RefPair<S, T> tryInvokeBind0(
ServiceTracker<T, RefPair<S, T>, ExtendedServiceEvent> tracker,
Object monitor, RefPair<S, T> refPair , int trackingCount)
{
@@ -894,6 +952,7 @@ public class DependencyManager<S, T> implements ReferenceManager<S, T>
if (invokeBind)
{
+ boolean invokedUnbind = false;
m_componentManager.invokeBindMethod(DependencyManager.this,
bindingRefPair, trackingCount);
if (!bindingRefPair.isFailed())
@@ -901,33 +960,30 @@ public class DependencyManager<S, T> implements ReferenceManager<S, T>
if (current != null)
{
m_componentManager.invokeUnbindMethod(DependencyManager.this,
- current,
- trackingCount);
+ current, trackingCount);
+ invokedUnbind = true;
ungetService(current);
}
}
else if (cardinalitySatisfied(0))
{
- m_componentManager.registerMissingDependency(DependencyManager.this, bindingRefPair.getRef(),
- trackingCount);
+ m_componentManager.registerMissingDependency(DependencyManager.this,
+ bindingRefPair.getRef(), trackingCount);
}
RefPair<S, T> next = null;
- synchronized (monitor) {
+ synchronized (monitor)
+ {
if (!queuedRefPairs.isEmpty())
{
// One of more threads offered better options
// Find the best option
- for (RefPair<S, T> betterCandidate : queuedRefPairs)
+ next = getBestFromQueue();
+ this.bindingRefPair = next;
+ if (invokedUnbind)
{
- if (next == null
- || betterCandidate.getRef().compareTo(next.getRef()) > 0)
- {
- next = betterCandidate;
- }
+ // unbind was invoked clear the current RefPair
+ this.currentRefPair = null;
}
- queuedRefPairs.clear();
- this.bindingRefPair = next;
- this.currentRefPair = null;
}
else
{
@@ -935,13 +991,19 @@ public class DependencyManager<S, T> implements ReferenceManager<S, T>
{
// what we just bound got unregistered
// look for next best service
- next = getTracker().getTracked(null,
- new AtomicInteger()).values().iterator().next();
+ Iterator<RefPair<S, T>> iNext = getTracker().getTracked(null,
+ new AtomicInteger()).values().iterator();
+ next = iNext.hasNext() ? iNext.next() : null;
this.bindingRefPair = next;
- this.currentRefPair = null;
+ if (invokedUnbind)
+ {
+ // unbind was invoked clear the current RefPair
+ this.currentRefPair = null;
+ }
}
else
{
+ // all done; set the current to the ref bound
this.currentRefPair = bindingRefPair;
this.bindingRefPair = null;
this.bindingThread = null;
@@ -1032,10 +1094,7 @@ public class DependencyManager<S, T> implements ReferenceManager<S, T>
}
if (nextRefPair != null)
{
- RefPair<S, T> next = nextRefPair;
- while ((next = tryinvokeBind(tracker, monitor, next, trackingCount)) != null)
- {
- }
+ tryInvokeBind(tracker, monitor, nextRefPair, trackingCount);
}
if (oldRefPair != null)
diff --git a/scr/src/test/java/org/apache/felix/scr/integration/ServiceBindGreedyTest.java b/scr/src/test/java/org/apache/felix/scr/integration/ServiceBindGreedyTest.java
index 5178dda..34d0298 100644
--- a/scr/src/test/java/org/apache/felix/scr/integration/ServiceBindGreedyTest.java
+++ b/scr/src/test/java/org/apache/felix/scr/integration/ServiceBindGreedyTest.java
@@ -25,6 +25,7 @@ import java.util.HashSet;
import java.util.Hashtable;
import java.util.LinkedList;
import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicReference;
import org.apache.felix.scr.integration.components.SimpleComponent;
@@ -52,7 +53,7 @@ public class ServiceBindGreedyTest extends ComponentTestBase
static
{
// uncomment to enable debugging of this test class
- // paxRunnerVmOption = DEBUG_VM_OPTION;
+ //paxRunnerVmOption = DEBUG_VM_OPTION;
descriptorFile = "/integration_test_simple_components_service_binding_greedy.xml";
}
@@ -172,18 +173,35 @@ public class ServiceBindGreedyTest extends ComponentTestBase
}
@Test
- public void test_optional_single_dynamic_multi_thread() throws Exception
+ public void test_optional_single_dynamic_multi_thread1() throws Exception
{
+ do_test_optional_single_dynamic_multi_thread(false);
+ }
+
+ @Test
+ public void test_optional_single_dynamic_multi_thread2() throws Throwable
+ {
+ do_test_optional_single_dynamic_multi_thread(true);
+ }
+
+ AtomicBoolean enabledConfig = new AtomicBoolean();
+ private void do_test_optional_single_dynamic_multi_thread(
+ Boolean unregPreExistingFirst)
+ throws Exception
+ {
+ String name = "test_optional_single_dynamic";
final int numRanks = 1000;
final int ranksPerThread = 100;
- final SimpleServiceImpl srv1 = SimpleServiceImpl.create(bundleContext, "srv1",
- numRanks / 4);
+ final SimpleServiceImpl preExistingServ = SimpleServiceImpl.create(bundleContext,
+ "preExistingServ", numRanks / 4);
- String name = "test_optional_single_dynamic";
- getDisabledConfigurationAndEnable(name, ComponentConfigurationDTO.ACTIVE);
+ if (enabledConfig.compareAndSet(false, true))
+ {
+ getDisabledConfigurationAndEnable(name, ComponentConfigurationDTO.ACTIVE);
+ }
final SimpleComponent comp = SimpleComponent.INSTANCE;
TestCase.assertNotNull(comp);
- TestCase.assertEquals(srv1, comp.m_singleRef);
+ TestCase.assertEquals(preExistingServ, comp.m_singleRef);
TestCase.assertTrue(comp.m_multiRef.isEmpty());
final List<Integer> ranks = Collections.synchronizedList(
@@ -200,12 +218,11 @@ public class ServiceBindGreedyTest extends ComponentTestBase
final AtomicReference<SimpleServiceImpl> highest = new AtomicReference<>();
for (int i = 0; i < numRanks; i = i + ranksPerThread)
{
- final int start = i;
threads.add(new Thread(new Runnable()
{
public void run()
{
- for (int j = start; j < start + ranksPerThread; j++)
+ for (int j = 0; j < ranksPerThread; j++)
{
int rank = ranks.remove(0);
SimpleServiceImpl srv = SimpleServiceImpl.create(bundleContext,
@@ -235,18 +252,21 @@ public class ServiceBindGreedyTest extends ComponentTestBase
threads.clear();
for (int i = 0; i < numRanks; i = i + ranksPerThread)
{
- final int start = i;
threads.add(new Thread(new Runnable()
{
public void run()
{
- for (int j = start; j < start + ranksPerThread; j++)
+ for (int j = 0; j < ranksPerThread; j++)
{
registrations.remove(0).drop();
}
};
}));
}
+ if (unregPreExistingFirst)
+ {
+ preExistingServ.drop();
+ }
for (Thread t : threads)
{
t.start();
@@ -255,11 +275,15 @@ public class ServiceBindGreedyTest extends ComponentTestBase
{
t.join();
}
- TestCase.assertEquals(srv1, comp.m_singleRef);
- TestCase.assertTrue(comp.m_multiRef.isEmpty());
+ if (!unregPreExistingFirst)
+ {
+ TestCase.assertEquals(preExistingServ, comp.m_singleRef);
+ TestCase.assertTrue(comp.m_multiRef.isEmpty());
+ preExistingServ.drop();
+ }
- srv1.drop();
- TestCase.assertEquals(null, comp.m_singleRef);
+ TestCase.assertEquals("Found bound reference: " + comp.m_singleRefBind + '-'
+ + comp.m_singleRefUnbind, null, comp.m_singleRef);
TestCase.assertTrue(comp.m_multiRef.isEmpty());
}
diff --git a/scr/src/test/java/org/apache/felix/scr/integration/components/SimpleComponent.java b/scr/src/test/java/org/apache/felix/scr/integration/components/SimpleComponent.java
index 886d9c4..4d697fa 100644
--- a/scr/src/test/java/org/apache/felix/scr/integration/components/SimpleComponent.java
+++ b/scr/src/test/java/org/apache/felix/scr/integration/components/SimpleComponent.java
@@ -41,7 +41,7 @@ public class SimpleComponent
public static final Set<SimpleComponent> PREVIOUS_INSTANCES = new HashSet<SimpleComponent>();
- public static SimpleComponent INSTANCE;
+ public volatile static SimpleComponent INSTANCE;
private Map<?, ?> m_config;
@@ -49,11 +49,11 @@ public class SimpleComponent
public ComponentContext m_activateContext;
- public SimpleService m_singleRef;
+ public volatile SimpleService m_singleRef;
- public int m_singleRefBind = 0;
+ public volatile int m_singleRefBind = 0;
- public int m_singleRefUnbind = 0;
+ public volatile int m_singleRefUnbind = 0;
public final Set<SimpleService> m_multiRef = new HashSet<SimpleService>();
diff --git a/scr/src/test/java/org/apache/felix/scr/integration/components/SimpleServiceImpl.java b/scr/src/test/java/org/apache/felix/scr/integration/components/SimpleServiceImpl.java
index 090902b..d365e6e 100644
--- a/scr/src/test/java/org/apache/felix/scr/integration/components/SimpleServiceImpl.java
+++ b/scr/src/test/java/org/apache/felix/scr/integration/components/SimpleServiceImpl.java
@@ -21,14 +21,13 @@ package org.apache.felix.scr.integration.components;
import java.util.Dictionary;
import java.util.Hashtable;
-import java.util.Properties;
import org.osgi.framework.BundleContext;
import org.osgi.framework.Constants;
import org.osgi.framework.ServiceRegistration;
-public class SimpleServiceImpl implements SimpleService
+public class SimpleServiceImpl implements SimpleService, Comparable<SimpleServiceImpl>
{
private String m_value;
@@ -37,7 +36,7 @@ public class SimpleServiceImpl implements SimpleService
private String m_filterProp;
- private ServiceRegistration m_registration;
+ private ServiceRegistration<SimpleService> m_registration;
public static SimpleServiceImpl create( BundleContext bundleContext, String value )
@@ -50,7 +49,8 @@ public class SimpleServiceImpl implements SimpleService
{
SimpleServiceImpl instance = new SimpleServiceImpl( value, ranking );
Dictionary<String,?> props = instance.getProperties();
- instance.setRegistration( bundleContext.registerService( SimpleService.class.getName(), instance, props ) );
+ instance.setRegistration(
+ bundleContext.registerService(SimpleService.class, instance, props));
return instance;
}
@@ -104,7 +104,7 @@ public class SimpleServiceImpl implements SimpleService
public SimpleServiceImpl drop()
{
- ServiceRegistration sr = getRegistration();
+ ServiceRegistration<SimpleService> sr = getRegistration();
if ( sr != null )
{
setRegistration( null );
@@ -120,14 +120,15 @@ public class SimpleServiceImpl implements SimpleService
}
- public SimpleServiceImpl setRegistration( ServiceRegistration registration )
+ public SimpleServiceImpl setRegistration(
+ ServiceRegistration<SimpleService> registration)
{
m_registration = registration;
return this;
}
- public ServiceRegistration getRegistration()
+ public ServiceRegistration<SimpleService> getRegistration()
{
return m_registration;
}
@@ -136,6 +137,13 @@ public class SimpleServiceImpl implements SimpleService
@Override
public String toString()
{
- return getClass().getSimpleName() + ": value=" + getValue() + ", filterprop=" + m_filterProp;
+ return getClass().getSimpleName() + ": value=" + getValue() + ", filterprop="
+ + m_filterProp + ", m_registration=" + m_registration;
+ }
+
+ @Override
+ public int compareTo(SimpleServiceImpl o)
+ {
+ return Integer.compare(this.m_ranking, o.m_ranking);
}
}