Bug 553773 - Avoid calling hooks while registering
This avoids getting the framework hook until after it has finished
registering. Some hooks run into issues if they are allowed to be
called while they are registering. For example, WeavingHooks that are
registered by declarative services can cause issues with re-entering
into activation by SCR.
This change also avoids the constant get/unget service hooks
registrations each time the hook is invoked. Hooks are high traffic
services during event storms. This should improve performance when
hooks are present.
Change-Id: Ic937616d421f1d5944b614a91e74417070f06fd0
Signed-off-by: Thomas Watson <tjwatson@us.ibm.com>
diff --git a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/ClassLoadingBundleTests.java b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/ClassLoadingBundleTests.java
index 2f509b2..f279c8c 100644
--- a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/ClassLoadingBundleTests.java
+++ b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/ClassLoadingBundleTests.java
@@ -1,5 +1,5 @@
/*******************************************************************************
- * Copyright (c) 2006, 2018 IBM Corporation and others.
+ * Copyright (c) 2006, 2020 IBM Corporation and others.
*
* This program and the accompanying materials
* are made available under the terms of the Eclipse Public License 2.0
@@ -2283,7 +2283,7 @@
reg.unregister();
}
- assertEquals("Unexpected number of woven classes.", 3, called.size());
+ assertEquals("Unexpected number of woven classes.", 2, called.size());
for (WovenClass wovenClass : called) {
if (weavingHookClasses.contains(wovenClass.getClassName())) {
assertNull("Did not expect to find class: " + wovenClass.getDefinedClass(), wovenClass.getDefinedClass());
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/serviceregistry/ServiceRegistrationImpl.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/serviceregistry/ServiceRegistrationImpl.java
index c81a64b..0d1e51e 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/serviceregistry/ServiceRegistrationImpl.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/serviceregistry/ServiceRegistrationImpl.java
@@ -234,6 +234,7 @@
}
}
+ ungetHookInstance();
/* must not hold the registrationLock when this event is published */
registry.publishServiceEvent(new ServiceEvent(ServiceEvent.UNREGISTERING, ref));
@@ -291,6 +292,18 @@
return getReferenceImpl();
}
+ S getHookInstance() {
+ return null;
+ }
+
+ void initHookInstance() {
+ // nothing by default
+ }
+
+ void ungetHookInstance() {
+ // nothing by default
+ }
+
ServiceReferenceImpl<S> getReferenceImpl() {
/* use reference instead of unregistered so that ServiceFactorys, called
* by releaseService after the registration is unregistered, can
@@ -759,4 +772,46 @@
}
return 1;
}
+
+ static class FrameworkHookRegistration<S> extends ServiceRegistrationImpl<S> {
+ private volatile boolean hookInitialized = false;
+ private volatile S hookInstance;
+ private final BundleContextImpl systemContext;
+ private final Object hookLock = new Object();
+
+ FrameworkHookRegistration(ServiceRegistry registry, BundleContextImpl context, String[] clazzes, S service,
+ BundleContextImpl systemContext) {
+ super(registry, context, clazzes, service);
+ this.systemContext = systemContext;
+ }
+
+ @Override
+ S getHookInstance() {
+ if (hookInstance != null || !hookInitialized) {
+ return hookInstance;
+ }
+ synchronized (hookLock) {
+ if (hookInstance == null) {
+ hookInstance = getSafeService(systemContext, ServiceConsumer.singletonConsumer);
+ }
+ }
+ return hookInstance;
+ }
+
+ @Override
+ void initHookInstance() {
+ ServiceReference<S> ref = getReference();
+ if (ref != null) {
+ hookInstance = getSafeService(systemContext, ServiceConsumer.singletonConsumer);
+ hookInitialized = true;
+ }
+ }
+
+ @Override
+ void ungetHookInstance() {
+ if (hookInstance != null) {
+ systemContext.ungetService(getReferenceImpl());
+ }
+ }
+ }
}
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/serviceregistry/ServiceRegistry.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/serviceregistry/ServiceRegistry.java
index 36ceb97..934cc0c 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/serviceregistry/ServiceRegistry.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/serviceregistry/ServiceRegistry.java
@@ -1,5 +1,5 @@
/*******************************************************************************
- * Copyright (c) 2004, 2017 IBM Corporation and others.
+ * Copyright (c) 2004, 2020 IBM Corporation and others.
*
* This program and the accompanying materials
* are made available under the terms of the Eclipse Public License 2.0
@@ -74,6 +74,7 @@
static final String eventListenerHookName = EventListenerHook.class.getName();
static final String listenerHookName = ListenerHook.class.getName();
+
/** Published services by class name.
* The {@literal List<ServiceRegistrationImpl<?>>}s are both sorted
* in the natural order of ServiceRegistrationImpl and also are sets in that
@@ -229,12 +230,16 @@
throw new IllegalArgumentException(Msg.SERVICE_EMPTY_CLASS_LIST_EXCEPTION);
}
+ boolean isListenerHook = false;
+ boolean isFrameworkHook = false;
/* copy the array so that changes to the original will not affect us. */
List<String> copy = new ArrayList<>(size);
// intern the strings and remove duplicates
for (int i = 0; i < size; i++) {
String clazz = clazzes[i].intern();
if (!copy.contains(clazz)) {
+ isListenerHook = isListenerHook || listenerHookName.equals(clazz);
+ isFrameworkHook = isFrameworkHook || isFrameworkHook(clazz);
copy.add(clazz);
}
}
@@ -254,14 +259,44 @@
}
}
- ServiceRegistrationImpl<?> registration = new ServiceRegistrationImpl<>(this, context, clazzes, service);
+ ServiceRegistrationImpl<?> registration = isFrameworkHook
+ ? new ServiceRegistrationImpl.FrameworkHookRegistration<>(this, context, clazzes, service,
+ systemBundleContext)
+ : new ServiceRegistrationImpl<>(this, context, clazzes, service);
registration.register(properties);
- if (copy.contains(listenerHookName)) {
+ registration.initHookInstance();
+
+ if (isListenerHook) {
notifyNewListenerHook(registration);
}
return registration;
}
+ private boolean isFrameworkHook(String className) {
+ switch (className) {
+ case "org.osgi.framework.hooks.bundle.CollisionHook": //$NON-NLS-1$
+ return true;
+ case "org.osgi.framework.hooks.bundle.EventHook": //$NON-NLS-1$
+ return true;
+ case "org.osgi.framework.hooks.bundle.FindHook": //$NON-NLS-1$
+ return true;
+ case "org.osgi.framework.hooks.service.EventHook": //$NON-NLS-1$
+ return true;
+ case "org.osgi.framework.hooks.service.EventListenerHook": //$NON-NLS-1$
+ return true;
+ case "org.osgi.framework.hooks.service.FindHook": //$NON-NLS-1$
+ return true;
+ case "org.osgi.framework.hooks.service.ListenerHook": //$NON-NLS-1$
+ return true;
+ case "org.osgi.framework.hooks.weaving.WeavingHook": //$NON-NLS-1$
+ return true;
+ case "org.osgi.framework.hooks.weaving.WovenClassListener": //$NON-NLS-1$
+ return true;
+ default:
+ return false;
+ }
+ }
+
/**
* Returns an array of <code>ServiceReferenceImpl</code> objects. The returned
* array of <code>ServiceReferenceImpl</code> objects contains services that
@@ -1335,8 +1370,11 @@
if (hookContext.skipRegistration(registration)) {
return;
}
- Object hook = registration.getSafeService(context, ServiceConsumer.singletonConsumer);
- if (hook == null) { // if the hook is null
+ Object hook = registration.getHookInstance();
+ if (hook == null) {
+ // The hook may not be initialized yet
+ // We do not call the hook until after it has been registered
+ // This means we could miss calls to a hook during the registered event.
return;
}
try {
@@ -1350,8 +1388,6 @@
container.handleRuntimeError(t);
ServiceException se = new ServiceException(NLS.bind(Msg.SERVICE_FACTORY_EXCEPTION, hook.getClass().getName(), hookContext.getHookMethodName()), t);
container.getEventPublisher().publishFrameworkEvent(FrameworkEvent.ERROR, registration.getBundle(), se);
- } finally {
- registration.ungetService(context, ServiceConsumer.singletonConsumer, null);
}
}
