From 7a2d5cb49eab242e10233e0218ee1906c13df901 Mon Sep 17 00:00:00 2001 From: Tom Pantelis Date: Thu, 1 Oct 2015 02:54:45 -0400 Subject: [PATCH] Revert ModuleInfoBundleTracker patches Reverted patch https://git.opendaylight.org/gerrit/#/c/27138/ as it causes some feature tests to take a long time due to the busy wait. Also it appears the ModuleFactory OSGi services are needed as the BlankTransactionServiceTracker listens for them (I'm not clear what this does). I'll try to figure out another way to accomplish the intent of the reverted patch. Change-Id: Ifc91dada86ac7feee1a0a9390a55e68d7f113153 Signed-off-by: Tom Pantelis --- ...eContextBackedModuleFactoriesResolver.java | 51 +++++--- .../impl/osgi/ConfigManagerActivator.java | 8 +- .../impl/osgi/ExtensibleBundleTracker.java | 15 ++- .../impl/osgi/ModuleFactoryBundleTracker.java | 115 ++---------------- ...textBackedModuleFactoriesResolverTest.java | 115 +++++------------- .../osgi/ModuleFactoryBundleTrackerTest.java | 49 ++------ 6 files changed, 96 insertions(+), 257 deletions(-) diff --git a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/BundleContextBackedModuleFactoriesResolver.java b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/BundleContextBackedModuleFactoriesResolver.java index e9af46819f..e8639d5881 100644 --- a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/BundleContextBackedModuleFactoriesResolver.java +++ b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/BundleContextBackedModuleFactoriesResolver.java @@ -8,12 +8,14 @@ package org.opendaylight.controller.config.manager.impl.osgi; import java.util.AbstractMap; +import java.util.Collection; import java.util.HashMap; import java.util.Map; -import java.util.Map.Entry; import org.opendaylight.controller.config.manager.impl.factoriesresolver.ModuleFactoriesResolver; import org.opendaylight.controller.config.spi.ModuleFactory; import org.osgi.framework.BundleContext; +import org.osgi.framework.InvalidSyntaxException; +import org.osgi.framework.ServiceReference; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -24,27 +26,42 @@ public class BundleContextBackedModuleFactoriesResolver implements ModuleFactoriesResolver { private static final Logger LOG = LoggerFactory .getLogger(BundleContextBackedModuleFactoriesResolver.class); - private ModuleFactoryBundleTracker moduleFactoryBundleTracker; + private final BundleContext bundleContext; - public BundleContextBackedModuleFactoriesResolver() { - } - - public void setModuleFactoryBundleTracker(ModuleFactoryBundleTracker moduleFactoryBundleTracker) { - this.moduleFactoryBundleTracker = moduleFactoryBundleTracker; + public BundleContextBackedModuleFactoriesResolver( + BundleContext bundleContext) { + this.bundleContext = bundleContext; } @Override public Map> getAllFactories() { - Map> result = new HashMap<>(); - for(Entry entry: moduleFactoryBundleTracker.getModuleFactoryEntries()) { - ModuleFactory factory = entry.getKey(); - BundleContext bundleContext = entry.getValue(); - String moduleName = factory .getImplementationName(); - if (moduleName == null || moduleName.isEmpty()) { - throw new IllegalStateException("Invalid implementation name for " + factory); + Collection> serviceReferences; + try { + serviceReferences = bundleContext.getServiceReferences( + ModuleFactory.class, null); + } catch (InvalidSyntaxException e) { + throw new IllegalStateException(e); + } + Map> result = new HashMap<>(serviceReferences.size()); + for (ServiceReference serviceReference : serviceReferences) { + ModuleFactory factory = bundleContext.getService(serviceReference); + // null if the service is not registered, the service object + // returned by a ServiceFactory does not + // implement the classes under which it was registered or the + // ServiceFactory threw an exception. + if(factory == null) { + throw new NullPointerException("ServiceReference of class" + serviceReference.getClass() + "not found."); } - LOG.debug("Processing factory {} {}", moduleName, factory); + String moduleName = factory.getImplementationName(); + if (moduleName == null || moduleName.isEmpty()) { + throw new IllegalStateException( + "Invalid implementation name for " + factory); + } + if (serviceReference.getBundle() == null || serviceReference.getBundle().getBundleContext() == null) { + throw new NullPointerException("Bundle context of " + factory + " ModuleFactory not found."); + } + LOG.debug("Reading factory {} {}", moduleName, factory); Map.Entry conflicting = result.get(moduleName); if (conflicting != null) { @@ -54,10 +71,10 @@ public class BundleContextBackedModuleFactoriesResolver implements LOG.error(error); throw new IllegalArgumentException(error); } else { - result.put(moduleName, new AbstractMap.SimpleImmutableEntry<>(factory, bundleContext)); + result.put(moduleName, new AbstractMap.SimpleImmutableEntry<>(factory, + serviceReference.getBundle().getBundleContext())); } } - return result; } } diff --git a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ConfigManagerActivator.java b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ConfigManagerActivator.java index ab52e8f143..128399563a 100644 --- a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ConfigManagerActivator.java +++ b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ConfigManagerActivator.java @@ -25,7 +25,6 @@ import org.opendaylight.controller.config.manager.impl.util.OsgiRegistrationUtil import org.opendaylight.controller.config.spi.ModuleFactory; import org.opendaylight.yangtools.sal.binding.generator.impl.GeneratedClassLoadingStrategy; import org.opendaylight.yangtools.sal.binding.generator.impl.ModuleInfoBackedContext; -import org.osgi.framework.Bundle; import org.osgi.framework.BundleActivator; import org.osgi.framework.BundleContext; import org.osgi.util.tracker.ServiceTracker; @@ -53,7 +52,8 @@ public class ConfigManagerActivator implements BundleActivator { ModuleInfoBundleTracker moduleInfoBundleTracker = new ModuleInfoBundleTracker(moduleInfoRegistryWrapper); // start config registry - BundleContextBackedModuleFactoriesResolver bundleContextBackedModuleFactoriesResolver = new BundleContextBackedModuleFactoriesResolver(); + BundleContextBackedModuleFactoriesResolver bundleContextBackedModuleFactoriesResolver = new BundleContextBackedModuleFactoriesResolver( + context); ConfigRegistryImpl configRegistry = new ConfigRegistryImpl(bundleContextBackedModuleFactoriesResolver, configMBeanServer, bindingContextProvider); @@ -62,12 +62,10 @@ public class ConfigManagerActivator implements BundleActivator { configRegistry); ModuleFactoryBundleTracker primaryModuleFactoryBundleTracker = new ModuleFactoryBundleTracker( blankTransactionServiceTracker); - bundleContextBackedModuleFactoriesResolver.setModuleFactoryBundleTracker(primaryModuleFactoryBundleTracker); // start extensible tracker ExtensibleBundleTracker bundleTracker = new ExtensibleBundleTracker<>(context, - Bundle.RESOLVED | Bundle.STARTING | Bundle.ACTIVE, - primaryModuleFactoryBundleTracker, moduleInfoBundleTracker); + primaryModuleFactoryBundleTracker, moduleInfoBundleTracker); bundleTracker.open(); // Wrap config registry with JMX notification publishing adapter diff --git a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ExtensibleBundleTracker.java b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ExtensibleBundleTracker.java index 61c050506e..6d29604f3d 100644 --- a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ExtensibleBundleTracker.java +++ b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ExtensibleBundleTracker.java @@ -7,11 +7,13 @@ */ package org.opendaylight.controller.config.manager.impl.osgi; -import com.google.common.util.concurrent.MoreExecutors; +import com.google.common.util.concurrent.ThreadFactoryBuilder; import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import java.util.concurrent.Future; +import java.util.concurrent.ThreadFactory; import org.osgi.framework.Bundle; import org.osgi.framework.BundleContext; import org.osgi.framework.BundleEvent; @@ -47,7 +49,8 @@ import org.slf4j.LoggerFactory; * @param */ public final class ExtensibleBundleTracker extends BundleTracker> { - + private static final ThreadFactory THREAD_FACTORY = new ThreadFactoryBuilder() + .setNameFormat("config-bundle-tracker-%d").build(); private final ExecutorService eventExecutor; private final BundleTrackerCustomizer primaryTracker; private final BundleTrackerCustomizer[] additionalTrackers; @@ -55,17 +58,17 @@ public final class ExtensibleBundleTracker extends BundleTracker> { private static final Logger LOG = LoggerFactory.getLogger(ExtensibleBundleTracker.class); public ExtensibleBundleTracker(final BundleContext context, final BundleTrackerCustomizer primaryBundleTrackerCustomizer, - final BundleTrackerCustomizer... additionalBundleTrackerCustomizers) { + final BundleTrackerCustomizer... additionalBundleTrackerCustomizers) { this(context, Bundle.ACTIVE, primaryBundleTrackerCustomizer, additionalBundleTrackerCustomizers); } public ExtensibleBundleTracker(final BundleContext context, final int bundleState, - final BundleTrackerCustomizer primaryBundleTrackerCustomizer, - final BundleTrackerCustomizer... additionalBundleTrackerCustomizers) { + final BundleTrackerCustomizer primaryBundleTrackerCustomizer, + final BundleTrackerCustomizer... additionalBundleTrackerCustomizers) { super(context, bundleState, null); this.primaryTracker = primaryBundleTrackerCustomizer; this.additionalTrackers = additionalBundleTrackerCustomizers; - eventExecutor = MoreExecutors.newDirectExecutorService(); + eventExecutor = Executors.newSingleThreadExecutor(THREAD_FACTORY); LOG.trace("Registered as extender with context {} and bundle state {}", context, bundleState); } diff --git a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ModuleFactoryBundleTracker.java b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ModuleFactoryBundleTracker.java index 1ece5627e4..cd72a73ecf 100644 --- a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ModuleFactoryBundleTracker.java +++ b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ModuleFactoryBundleTracker.java @@ -10,24 +10,13 @@ package org.opendaylight.controller.config.manager.impl.osgi; import static java.lang.String.format; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Charsets; -import com.google.common.base.Stopwatch; -import com.google.common.collect.HashMultimap; -import com.google.common.collect.Multimap; import com.google.common.io.Resources; -import com.google.common.util.concurrent.Uninterruptibles; import java.io.IOException; import java.net.URL; -import java.util.AbstractMap; -import java.util.ArrayList; -import java.util.Collection; -import java.util.Map; -import java.util.Map.Entry; -import java.util.concurrent.TimeUnit; -import javax.annotation.concurrent.GuardedBy; import org.opendaylight.controller.config.spi.ModuleFactory; import org.osgi.framework.Bundle; -import org.osgi.framework.BundleContext; import org.osgi.framework.BundleEvent; +import org.osgi.framework.ServiceRegistration; import org.osgi.util.tracker.BundleTrackerCustomizer; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -42,13 +31,8 @@ import org.slf4j.LoggerFactory; * Code based on http://www.toedter.com/blog/?p=236 */ public class ModuleFactoryBundleTracker implements BundleTrackerCustomizer { - private static final Logger LOG = LoggerFactory.getLogger(ModuleFactoryBundleTracker.class); - private static final long BUNDLE_CONTEXT_TIMEOUT = TimeUnit.MILLISECONDS.convert(60, TimeUnit.SECONDS); - private final BlankTransactionServiceTracker blankTransactionServiceTracker; - - @GuardedBy(value = "bundleModuleFactoryMap") - private final Multimap bundleModuleFactoryMap = HashMultimap.create(); + private static final Logger LOG = LoggerFactory.getLogger(ModuleFactoryBundleTracker.class); public ModuleFactoryBundleTracker(BlankTransactionServiceTracker blankTransactionServiceTracker) { this.blankTransactionServiceTracker = blankTransactionServiceTracker; @@ -56,26 +40,13 @@ public class ModuleFactoryBundleTracker implements BundleTrackerCustomizer moduleFactoryEntry = registerFactory(factoryClassName, bundle); - synchronized (bundleModuleFactoryMap) { - bundleModuleFactoryMap.put(new BundleKey(moduleFactoryEntry.getValue()), - moduleFactoryEntry.getKey()); - } + registerFactory(factoryClassName, bundle); } } catch (IOException e) { LOG.error("Error while reading {}", resource, e); @@ -92,44 +63,23 @@ public class ModuleFactoryBundleTracker implements BundleTrackerCustomizer> getModuleFactoryEntries() { - Collection> entries; - synchronized (bundleModuleFactoryMap) { - entries = new ArrayList<>(bundleModuleFactoryMap.entries()); - } - - Collection> result = new ArrayList<>(entries.size()); - for(Entry entry: entries) { - BundleContext context = entry.getKey().getBundleContext(); - if(context == null) { - LOG.warn("Bundle context for {} ModuleFactory not found", entry.getValue()); - } else { - result.add(new AbstractMap.SimpleImmutableEntry<>(entry.getValue(), context)); - } - } - - return result; - } - @VisibleForTesting - protected static Map.Entry registerFactory(String factoryClassName, Bundle bundle) { + protected static ServiceRegistration registerFactory(String factoryClassName, Bundle bundle) { String errorMessage; Exception ex = null; try { Class clazz = bundle.loadClass(factoryClassName); if (ModuleFactory.class.isAssignableFrom(clazz)) { try { - LOG.debug("Registering {} in bundle {}", clazz.getName(), bundle); - - return new AbstractMap.SimpleImmutableEntry<>((ModuleFactory)clazz.newInstance(), bundle); + LOG.debug("Registering {} in bundle {}", + clazz.getName(), bundle); + return bundle.getBundleContext().registerService( + ModuleFactory.class.getName(), clazz.newInstance(), + null); } catch (InstantiationException e) { errorMessage = logMessage( "Could not instantiate {} in bundle {}, reason {}", @@ -140,11 +90,6 @@ public class ModuleFactoryBundleTracker implements BundleTrackerCustomizer s1; + private ServiceReference s2; private ModuleFactory f1; private ModuleFactory f2; @Before public void setUp() throws Exception { MockitoAnnotations.initMocks(this); - doReturn(mockBundleContext1).when(mockBundle1).getBundleContext(); - doReturn(mockBundleContext2).when(mockBundle2).getBundleContext(); - + s1 = getServiceRef(); + s2 = getServiceRef(); + doReturn(Lists.newArrayList(s1, s2)).when(bundleContext).getServiceReferences(ModuleFactory.class, null); f1 = getMockFactory("f1"); + doReturn(f1).when(bundleContext).getService(s1); f2 = getMockFactory("f2"); - - resolver = new BundleContextBackedModuleFactoriesResolver(); - resolver.setModuleFactoryBundleTracker(mockBundleTracker); + doReturn(f2).when(bundleContext).getService(s2); + resolver = new BundleContextBackedModuleFactoriesResolver(bundleContext); } private ModuleFactory getMockFactory(final String name) { @@ -74,28 +56,30 @@ public class BundleContextBackedModuleFactoriesResolverTest { return mock; } + private ServiceReference getServiceRef() { + ServiceReference mock = mock(ServiceReference.class); + doReturn("serviceRef").when(mock).toString(); + final Bundle bundle = mock(Bundle.class); + doReturn(bundleContext).when(bundle).getBundleContext(); + doReturn(bundle).when(mock).getBundle(); + return mock; + } + @Test public void testGetAllFactories() throws Exception { - doReturn(Arrays.asList(new AbstractMap.SimpleImmutableEntry<>(f1, mockBundleContext1), - new AbstractMap.SimpleImmutableEntry<>(f2, mockBundleContext2))). - when(mockBundleTracker).getModuleFactoryEntries(); - Map> allFactories = resolver.getAllFactories(); assertEquals(2, allFactories.size()); assertTrue(allFactories.containsKey(f1.getImplementationName())); assertEquals(f1, allFactories.get(f1.getImplementationName()).getKey()); - assertEquals(mockBundleContext1, allFactories.get(f1.getImplementationName()).getValue()); + assertEquals(bundleContext, allFactories.get(f1.getImplementationName()).getValue()); assertTrue(allFactories.containsKey(f2.getImplementationName())); assertEquals(f2, allFactories.get(f2.getImplementationName()).getKey()); - assertEquals(mockBundleContext2, allFactories.get(f2.getImplementationName()).getValue()); + assertEquals(bundleContext, allFactories.get(f2.getImplementationName()).getValue()); } @Test public void testDuplicateFactories() throws Exception { - doReturn(Arrays.asList(new AbstractMap.SimpleImmutableEntry<>(f1, mockBundleContext1), - new AbstractMap.SimpleImmutableEntry<>(f1, mockBundleContext2))). - when(mockBundleTracker).getModuleFactoryEntries(); - + doReturn(f1).when(bundleContext).getService(s2); try { resolver.getAllFactories(); } catch (Exception e) { @@ -107,54 +91,21 @@ public class BundleContextBackedModuleFactoriesResolverTest { fail("Should fail with duplicate factory name"); } + @Test(expected = NullPointerException.class) + public void testNullFactory() throws Exception { + doReturn(null).when(bundleContext).getService(s2); + resolver.getAllFactories(); + } + @Test(expected = IllegalStateException.class) public void testNullFactoryName() throws Exception { - doReturn(Arrays.asList(new AbstractMap.SimpleImmutableEntry<>(f1, mockBundleContext1))). - when(mockBundleTracker).getModuleFactoryEntries(); - doReturn(null).when(f1).getImplementationName(); resolver.getAllFactories(); } - @Test - public void testBundleContextInitiallyNull() throws Exception { - final AtomicReference bundleContext = new AtomicReference<>(); - Answer answer = new Answer() { - @Override - public BundleContext answer(InvocationOnMock invocation) throws Throwable { - return bundleContext.get(); - } - }; - - doAnswer(answer).when(mockBundle1).getBundleContext(); - doReturn(Arrays.asList(new AbstractMap.SimpleImmutableEntry<>(f1, mockBundleContext1))). - when(mockBundleTracker).getModuleFactoryEntries(); - - final AtomicReference>> allFactories = new AtomicReference<>(); - final AtomicReference caughtEx = new AtomicReference<>(); - final CountDownLatch doneLatch = new CountDownLatch(1); - new Thread() { - @Override - public void run() { - try { - allFactories.set(resolver.getAllFactories()); - } catch (Exception e) { - caughtEx.set(e); - } finally { - doneLatch.countDown(); - } - } - }.start(); - - Uninterruptibles.sleepUninterruptibly(500, TimeUnit.MILLISECONDS); - bundleContext.set(mockBundleContext1); - - assertEquals(true, doneLatch.await(5, TimeUnit.SECONDS)); - if(caughtEx.get() != null) { - throw caughtEx.get(); - } - - assertEquals(1, allFactories.get().size()); - assertTrue(allFactories.get().containsKey(f1.getImplementationName())); + @Test(expected = NullPointerException.class) + public void testNullBundleName() throws Exception { + doReturn(null).when(s1).getBundle(); + resolver.getAllFactories(); } } diff --git a/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/impl/osgi/ModuleFactoryBundleTrackerTest.java b/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/impl/osgi/ModuleFactoryBundleTrackerTest.java index da585b7151..9930a887c5 100644 --- a/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/impl/osgi/ModuleFactoryBundleTrackerTest.java +++ b/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/impl/osgi/ModuleFactoryBundleTrackerTest.java @@ -15,15 +15,11 @@ import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyObject; import static org.mockito.Matchers.anyString; import static org.mockito.Mockito.doAnswer; -import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.reset; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyZeroInteractions; -import java.util.Collection; import java.util.Dictionary; -import java.util.Map.Entry; import java.util.Set; import org.junit.Before; import org.junit.Test; @@ -50,8 +46,6 @@ public class ModuleFactoryBundleTrackerTest { private BundleContext context; @Mock private ServiceRegistration reg; - @Mock - private BlankTransactionServiceTracker blankTxTracker; @Before public void setUp() throws Exception { @@ -63,18 +57,14 @@ public class ModuleFactoryBundleTrackerTest { } }).when(bundle).loadClass(anyString()); doReturn("mockBundle").when(bundle).toString(); - doReturn("mockBundleContext").when(context).toString(); doReturn(context).when(bundle).getBundleContext(); - doReturn(100L).when(bundle).getBundleId(); doReturn(reg).when(context).registerService(anyString(), anyObject(), any(Dictionary.class)); } @Test public void testRegisterFactory() throws Exception { - Entry entry = ModuleFactoryBundleTracker.registerFactory( - TestingFactory.class.getName(), bundle); - assertEquals(TestingFactory.currentInstance, entry.getKey()); - assertEquals(bundle, entry.getValue()); + ModuleFactoryBundleTracker.registerFactory(TestingFactory.class.getName(), bundle); + verify(context).registerService(ModuleFactory.class.getName(), TestingFactory.currentInstance, null); } @Test @@ -131,38 +121,15 @@ public class ModuleFactoryBundleTrackerTest { fail("Cannot register without extend"); } + @Mock + private BlankTransactionServiceTracker blankTxTracker; + @Test - public void testBundleAddAndRemove() throws Exception { + public void testAddingBundle() throws Exception { final ModuleFactoryBundleTracker tracker = new ModuleFactoryBundleTracker(blankTxTracker); doReturn(getClass().getResource("/module-factories/module-factory-ok")).when(bundle).getEntry(anyString()); - tracker.addingBundle(bundle, null); - - Collection> entries = tracker.getModuleFactoryEntries(); - assertNotNull(entries); - assertEquals(1, entries.size()); - Entry entry = entries.iterator().next(); - assertEquals(TestingFactory.currentInstance, entry.getKey()); - assertEquals(context, entry.getValue()); - - doNothing().when(blankTxTracker).blankTransaction();; - - BundleEvent mockEvent = mock(BundleEvent.class); - doReturn(BundleEvent.STOPPING).when(mockEvent).getType(); - - tracker.removedBundle(bundle, mockEvent, bundle); - - entries = tracker.getModuleFactoryEntries(); - assertNotNull(entries); - assertEquals(0, entries.size()); - - verify(blankTxTracker).blankTransaction(); - - reset(mockEvent); - doReturn(BundleEvent.STOPPED).when(mockEvent).getType(); - - tracker.addingBundle(bundle, mockEvent); - - assertEquals(0, tracker.getModuleFactoryEntries().size()); + tracker.addingBundle(bundle, mock(BundleEvent.class)); + verify(context).registerService(ModuleFactory.class.getName(), TestingFactory.currentInstance, null); } @Test -- 2.36.6