diff options
author | Thomas Watson | 2020-05-15 14:02:05 +0000 |
---|---|---|
committer | Thomas Watson | 2020-05-15 21:10:49 +0000 |
commit | 01ffcf50611440654461f402bb1dd3008477f531 (patch) | |
tree | 708ef6fd3f3e6173720b378b0d5906927966d9a9 | |
parent | 638d4556eb9f14b36f3aff06cb7566d48e3ec1eb (diff) | |
download | rt.equinox.framework-01ffcf50611440654461f402bb1dd3008477f531.tar.gz rt.equinox.framework-01ffcf50611440654461f402bb1dd3008477f531.tar.xz rt.equinox.framework-01ffcf50611440654461f402bb1dd3008477f531.zip |
Bug 553773 - Avoid calling hooks while registeringI20200516-0600I20200515-1930
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>
3 files changed, 100 insertions, 9 deletions
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 2f509b25d..f279c8c3c 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 @@ public class ClassLoadingBundleTests extends AbstractBundleTests { 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 c81a64bfd..0d1e51e2a 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 @@ public class ServiceRegistrationImpl<S> implements ServiceRegistration<S>, Compa } } + ungetHookInstance(); /* must not hold the registrationLock when this event is published */ registry.publishServiceEvent(new ServiceEvent(ServiceEvent.UNREGISTERING, ref)); @@ -291,6 +292,18 @@ public class ServiceRegistrationImpl<S> implements ServiceRegistration<S>, Compa 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 @@ public class ServiceRegistrationImpl<S> implements ServiceRegistration<S>, Compa } 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 36ceb9790..934cc0cc6 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 @@ public class ServiceRegistry { 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 @@ public class ServiceRegistry { 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 @@ public class ServiceRegistry { } } - 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 @@ public class ServiceRegistry { 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 @@ public class ServiceRegistry { 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); } } |