518934 Deadlock when exporting service due to target bean caching

Signed-off-by: ootto <olaf@x100.de>
diff --git a/core/src/main/java/org/eclipse/gemini/blueprint/service/exporter/support/internal/support/LazyTargetResolver.java b/core/src/main/java/org/eclipse/gemini/blueprint/service/exporter/support/internal/support/LazyTargetResolver.java
index c1248f7..6f8f1cf 100644
--- a/core/src/main/java/org/eclipse/gemini/blueprint/service/exporter/support/internal/support/LazyTargetResolver.java
+++ b/core/src/main/java/org/eclipse/gemini/blueprint/service/exporter/support/internal/support/LazyTargetResolver.java
@@ -7,117 +7,144 @@
  * http://www.eclipse.org/legal/epl-v10.html and the Apache License v2.0

  * is available at http://www.opensource.org/licenses/apache2.0.php.

  * You may elect to redistribute this code under either of these licenses. 

- * 

+ *

  * Contributors:

  *   VMware Inc.

  *****************************************************************************/

 

 package org.eclipse.gemini.blueprint.service.exporter.support.internal.support;

 

-import java.util.Map;

-import java.util.concurrent.atomic.AtomicBoolean;

-

 import org.eclipse.gemini.blueprint.util.OsgiServiceReferenceUtils;

 import org.springframework.beans.factory.BeanFactory;

 

+import java.util.Map;

+import java.util.Optional;

+import java.util.concurrent.atomic.AtomicBoolean;

+

+import static java.util.Optional.empty;

+import static java.util.Optional.of;

+

 /**

  * Class encapsulating the lazy dependency lookup of the target bean. Handles caching as well as the lazy listener

  * notifications.

- * 

+ *

  * @author Costin Leau

  */

 public class LazyTargetResolver implements UnregistrationNotifier {

 

-	private final BeanFactory beanFactory;

-	private final String beanName;

-	private final boolean cacheService;

-	private volatile Object target;

-	private final Object lock = new Object();

-	private final AtomicBoolean activated;

-	private final ListenerNotifier notifier;

-	private volatile ServiceRegistrationDecorator decorator;

+    private final BeanFactory beanFactory;

+    private final String beanName;

+    private final boolean cacheService;

+    private volatile Object target;

+    private final Object lock = new Object();

+    private final AtomicBoolean activated;

+    private final ListenerNotifier notifier;

+    private volatile ServiceRegistrationDecorator decorator;

 

-	public LazyTargetResolver(Object target, BeanFactory beanFactory, String beanName, boolean cacheService,

-			ListenerNotifier notifier, boolean lazyListeners) {

-		this.target = target;

-		this.beanFactory = beanFactory;

-		this.beanName = beanName;

-		this.cacheService = cacheService;

-		this.notifier = notifier;

-		this.activated = new AtomicBoolean(!lazyListeners);

-	}

+    public LazyTargetResolver(Object target, BeanFactory beanFactory, String beanName, boolean cacheService,

+                              ListenerNotifier notifier, boolean lazyListeners) {

+        this.target = target;

+        this.beanFactory = beanFactory;

+        this.beanName = beanName;

+        this.cacheService = cacheService;

+        this.notifier = notifier;

+        this.activated = new AtomicBoolean(!lazyListeners);

+    }

 

-	public void activate() {

-		if (activated.compareAndSet(false, true) && notifier != null) {

-			// no service registered

-			if (decorator == null) {

-				notifier.callUnregister(null, null);

-			} else {

-				Object target = getBeanIfPossible();

-				Map properties = (Map) OsgiServiceReferenceUtils.getServicePropertiesSnapshot(decorator.getReference());

-				notifier.callRegister(target, properties);

-			}

-		}

-	}

+    public void activate() {

+        if (activated.compareAndSet(false, true) && notifier != null) {

+            // no service registered

+            if (decorator == null) {

+                notifier.callUnregister(null, null);

+            } else {

+                Object target = getBeanIfAlreadyInstantiatedOrSingletonScoped().orElse(null);

+                Map properties = (Map) OsgiServiceReferenceUtils.getServicePropertiesSnapshot(decorator.getReference());

+                notifier.callRegister(target, properties);

+            }

+        }

+    }

 

-	private Object getBeanIfPossible() {

-		if (target == null) {

-			if (cacheService || beanFactory.isSingleton(beanName)) {

-				getBean();

-			}

-		}

+    public Object getBean() {

+        if (target != null) {

+            return target;

+        }

 

-		return target;

-	}

+        if (beanFactory.isSingleton(beanName) || !cacheService) {

+            return beanFactory.getBean(beanName);

+        }

 

-	public Object getBean() {

-		if (target == null) {

-			if (cacheService) {

-				synchronized (lock) {

-					if (target == null) {

-						target = beanFactory.getBean(beanName);

-					}

-				}

-			} else {

-				return beanFactory.getBean(beanName);

-			}

-		}

-		return target;

-	}

+        // Per Blueprint spec, 121.6.8 Scope, a service must be represented by a single component instance,

+        // regardless of the component's scope. We must thus obtain the non-singleton component instance and treat

+        // it like a singleton.

+        //

+        // However, multiple bean instances may exist and may be requested concurrently in this scenario. Following, we will explicitly allow multiple

+        // bean instances to be created during concurrent access to avoid interlocking (independent locking here and in the bean factory) which would

+        // always mean potential deadlocks.

+        //

+        // The bean may be created multiple times, however the guarantees of the OSGi spec are met: Exactly one instance

+        // is shared as the service instance, superfluous instances are simply ignored.

+        Object targetCandidate = beanFactory.getBean(beanName);

+        if (target == null) {

+            synchronized (lock) {

+                if (target == null) {

+                    target = targetCandidate;

+                }

+            }

+        }

 

-	public Class<?> getType() {

-		return (target == null ? (beanFactory.isSingleton(beanName) ? beanFactory.getBean(beanName).getClass()

-				: beanFactory.getType(beanName)) : target.getClass());

+        return target;

+    }

 

-	}

+    public Class<?> getType() {

+        if (target != null) {

+            return target.getClass();

+        }

 

-	public void unregister(Map properties) {

-		if (activated.get() && notifier != null) {

-			Object target = getBeanIfPossible();

-			notifier.callUnregister(target, properties);

-		}

-	}

+        if (beanFactory.isSingleton(beanName)) {

+            return beanFactory.getBean(beanName).getClass();

+        }

 

-	public void setDecorator(ServiceRegistrationDecorator decorator) {

-		this.decorator = decorator;

-		if (decorator != null) {

-			decorator.setNotifier(this);

-		}

-	}

+        return beanFactory.getType(beanName);

+    }

 

-	public void notifyIfPossible() {

-		if (activated.get() && notifier != null) {

-			Object target = getBeanIfPossible();

-			Map properties = (Map) OsgiServiceReferenceUtils.getServicePropertiesSnapshot(decorator.getReference());

-			notifier.callRegister(target, properties);

-		}

-	}

+    public void unregister(Map properties) {

+        if (activated.get() && notifier != null) {

+            Object target = getBeanIfAlreadyInstantiatedOrSingletonScoped().orElse(null);

+            notifier.callUnregister(target, properties);

+        }

+    }

 

-	// called when the exporter is activated but no service is published

-	public void startupUnregisterIfPossible() {

-		if (activated.get() && notifier != null) {

-			notifier.callUnregister(null, null);

-		}

-	}

+    public void setDecorator(ServiceRegistrationDecorator decorator) {

+        this.decorator = decorator;

+        if (decorator != null) {

+            decorator.setNotifier(this);

+        }

+    }

+

+    public void notifyIfPossible() {

+        if (activated.get() && notifier != null) {

+            Object target = getBeanIfAlreadyInstantiatedOrSingletonScoped().orElse(null);

+            Map properties = (Map) OsgiServiceReferenceUtils.getServicePropertiesSnapshot(decorator.getReference());

+            notifier.callRegister(target, properties);

+        }

+    }

+

+    // called when the exporter is activated but no service is published

+

+    public void startupUnregisterIfPossible() {

+        if (activated.get() && notifier != null) {

+            notifier.callUnregister(null, null);

+        }

+    }

+

+    private Optional<Object> getBeanIfAlreadyInstantiatedOrSingletonScoped() {

+        if (target != null) {

+            return of(target);

+        }

+        if (cacheService || beanFactory.isSingleton(beanName)) {

+            return of(getBean());

+        }

+        return empty();

+    }

 }

 

diff --git a/core/src/test/java/org/eclipse/gemini/blueprint/config/OsgiServiceNamespaceHandlerTest.java b/core/src/test/java/org/eclipse/gemini/blueprint/config/OsgiServiceNamespaceHandlerTest.java
index 258e7c8..428e831 100644
--- a/core/src/test/java/org/eclipse/gemini/blueprint/config/OsgiServiceNamespaceHandlerTest.java
+++ b/core/src/test/java/org/eclipse/gemini/blueprint/config/OsgiServiceNamespaceHandlerTest.java
@@ -72,7 +72,7 @@
 

 		bundleContext = new MockBundleContext() {

 

-			public ServiceReference[] getServiceReferences(String clazz, String filter) throws InvalidSyntaxException {

+			public ServiceReference[] getServiceReferences(String clazz, String filter) {

 				return new ServiceReference[] { new MockServiceReference(new String[] { Cloneable.class.getName() }) };

 			}

 

@@ -99,7 +99,7 @@
 		return fact.getService(null, null);

 	}

 

-	public void testSimpleService() throws Exception {

+	public void testSimpleService() {

 		Object bean = appContext.getBean("&inlineReference");

 		assertSame(OsgiServiceFactoryBean.class, bean.getClass());

 		OsgiServiceFactoryBean exporter = (OsgiServiceFactoryBean) bean;

@@ -111,7 +111,7 @@
 		assertSame(appContext.getBean("string"), getServiceAtIndex(0));

 	}

 

-	public void testBiggerService() throws Exception {

+	public void testBiggerService() {

 		OsgiServiceFactoryBean exporter = (OsgiServiceFactoryBean) appContext.getBean("&manyOptions");

 

 		assertTrue(Arrays.equals(new Class<?>[] { Serializable.class, CharSequence.class }, getInterfaces(exporter)));

@@ -128,7 +128,7 @@
 		//assertEquals(appContext.getBean("string"), getTarget(exporter));

 	}

 

-	public void testNestedService() throws Exception {

+	public void testNestedService() {

 		OsgiServiceFactoryBean exporter = (OsgiServiceFactoryBean) appContext.getBean("&nestedService");

 		assertTrue(Arrays.equals(new Class<?>[] { Object.class }, getInterfaces(exporter)));

 

@@ -139,13 +139,13 @@
 		assertNotNull(getTarget(exporter));

 	}

 

-	public void testServiceExporterFactoryBean() throws Exception {

+	public void testServiceExporterFactoryBean() {

 		Object bean = appContext.getBean("nestedService");

 		assertTrue(bean instanceof ServiceRegistration);

 		assertNotSame("registration not wrapped to provide exporting listener notification", registration, bean);

 	}

 

-	public void testServiceProperties() throws Exception {

+	public void testServiceProperties() {

 		OsgiServiceFactoryBean exporter = (OsgiServiceFactoryBean) appContext.getBean("&serviceProperties");

 		Map properties = exporter.getServiceProperties();

 		assertEquals(2, properties.size());

@@ -157,7 +157,7 @@
 

 	}

 

-	public void testListeners() throws Exception {

+	public void testListeners() {

 		OsgiServiceFactoryBean exporter = (OsgiServiceFactoryBean) appContext.getBean("&exporterWithListener");

 		OsgiServiceRegistrationListener[] listeners = getListeners(exporter);

 		assertEquals(2, listeners.length);

@@ -178,7 +178,7 @@
 		assertSame(RegistrationListener.SERVICE_REG, RegistrationListener.SERVICE_UNREG);

 	}

 

-	public void testFBWithCustomListeners() throws Exception {

+	public void testFBWithCustomListeners() {

 		OsgiServiceFactoryBean exporter = (OsgiServiceFactoryBean) appContext.getBean("&exporterWithCustomListener");

 		OsgiServiceRegistrationListener[] listeners = getListeners(exporter);

 		assertEquals(1, listeners.length);

diff --git a/core/src/test/java/org/eclipse/gemini/blueprint/service/exporter/support/OsgiServiceFactoryBeanTest.java b/core/src/test/java/org/eclipse/gemini/blueprint/service/exporter/support/OsgiServiceFactoryBeanTest.java
index f909da4..4213ec6 100644
--- a/core/src/test/java/org/eclipse/gemini/blueprint/service/exporter/support/OsgiServiceFactoryBeanTest.java
+++ b/core/src/test/java/org/eclipse/gemini/blueprint/service/exporter/support/OsgiServiceFactoryBeanTest.java
@@ -295,7 +295,7 @@
         String beanName = "fooBar";
         exporter.setTargetBeanName(beanName);
         exporter.setInterfaces(new Class<?>[]{service.getClass()});
-        expect(beanFactory.isSingleton(beanName)).andReturn(true);
+        expect(beanFactory.isSingleton(beanName)).andReturn(true).times(2);
         expect(beanFactory.containsBean(beanName)).andReturn(true);
         expect(beanFactory.getBean(beanName)).andReturn(service);
         expect(beanFactory.getType(beanName)).andStubReturn((Class) service.getClass());
@@ -329,7 +329,7 @@
         };
 
         String beanName = "fooBar";
-        expect(beanFactory.isSingleton(beanName)).andReturn(true);
+        expect(beanFactory.isSingleton(beanName)).andReturn(true).times(2);
         expect(beanFactory.containsBean(beanName)).andReturn(true);
         expect(beanFactory.getBean(beanName)).andReturn(service);
         expect(beanFactory.getType(beanName)).andStubReturn((Class) service.getClass());