Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FELIX-6327 - NoSuchElementException when services are removed #47

Merged
merged 1 commit into from Sep 10, 2020
Merged

FELIX-6327 - NoSuchElementException when services are removed #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
111 changes: 85 additions & 26 deletions scr/src/main/java/org/apache/felix/scr/impl/manager/DependencyManager.java
View file
Open in desktop
Expand Up @@ -22,6 +22,7 @@
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;
Expand Down Expand Up @@ -840,10 +841,7 @@ public void addedService(ServiceReference<T> serviceReference, RefPair<S, T> ref
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))
Expand All @@ -863,7 +861,67 @@ else if (isTrackerOpened() && cardinalityJustSatisfied(serviceCount))
}
}

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)
{
Expand Down Expand Up @@ -894,54 +952,58 @@ private RefPair<S, T> tryinvokeBind(

if (invokeBind)
{
boolean invokedUnbind = false;
m_componentManager.invokeBindMethod(DependencyManager.this,
bindingRefPair, trackingCount);
if (!bindingRefPair.isFailed())
{
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
{
if (bindingRefPair.isDeleted())
{
// 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;
Expand Down Expand Up @@ -1032,10 +1094,7 @@ else if (!cardinalitySatisfied() && this.currentRefPair == null)
}
if (nextRefPair != null)
{
RefPair<S, T> next = nextRefPair;
while ((next = tryinvokeBind(tracker, monitor, next, trackingCount)) != null)
{
}
tryInvokeBind(tracker, monitor, nextRefPair, trackingCount);
}

if (oldRefPair != null)
Expand Down
54 changes: 39 additions & 15 deletions scr/src/test/java/org/apache/felix/scr/integration/ServiceBindGreedyTest.java
View file
Open in desktop
Expand Up @@ -25,6 +25,7 @@
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;
Expand Down Expand Up @@ -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";
}
Expand Down Expand Up @@ -172,18 +173,35 @@ public void test_optional_single_dynamic() throws Exception
}

@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(
Expand All @@ -200,12 +218,11 @@ public void test_optional_single_dynamic_multi_thread() throws Exception
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,
Expand Down Expand Up @@ -235,18 +252,21 @@ public void run()
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();
Expand All @@ -255,11 +275,15 @@ public void run()
{
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());
}

Expand Down
8 changes: 4 additions & 4 deletions scr/src/test/java/org/apache/felix/scr/integration/components/SimpleComponent.java
View file
Open in desktop
Expand Up @@ -41,19 +41,19 @@ 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;

public long m_id;

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>();

Expand Down
24 changes: 16 additions & 8 deletions scr/src/test/java/org/apache/felix/scr/integration/components/SimpleServiceImpl.java
View file
Open in desktop
Expand Up @@ -21,14 +21,13 @@

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;
Expand All @@ -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 )
Expand All @@ -50,7 +49,8 @@ public static SimpleServiceImpl create( BundleContext bundleContext, String valu
{
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;
}

Expand Down Expand Up @@ -104,7 +104,7 @@ public SimpleServiceImpl setFilterProperty( String filterProp )

public SimpleServiceImpl drop()
{
ServiceRegistration sr = getRegistration();
ServiceRegistration<SimpleService> sr = getRegistration();
if ( sr != null )
{
setRegistration( null );
Expand All @@ -120,14 +120,15 @@ public String getValue()
}


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;
}
Expand All @@ -136,6 +137,13 @@ public ServiceRegistration getRegistration()
@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);
}
}