From b5b811c0ef5cb509d5a2410c1c7ed76156dfb63b Mon Sep 17 00:00:00 2001 From: Thomas Watson Date: Fri, 15 Nov 2024 16:25:34 -0600 Subject: [PATCH] Stop throwing IllegalStateException The following methods can stop throwing IllegalStateExceptions Bundle.uninstall BundleContext.createFilter BundleContext.getBundle BundleContext.getProperty BundleContext.removeServiceListener BundleContext.removeBundleListener BundleContext.removeFrameworkListener BundleContext.ungetService ServiceObjects.ungetService ServiceRegistration.unregister --- .../META-INF/MANIFEST.MF | 2 +- bundles/org.eclipse.osgi.tests/pom.xml | 2 +- .../osgi/tests/bundles/BundleTests.java | 3 +- .../tests/bundles/ExceptionMessageTest.java | 24 --- .../bundles/IllegalStateExceptionTests.java | 138 ++++++++++++++++++ .../osgi/container/ModuleContainer.java | 5 +- .../internal/framework/BundleContextImpl.java | 17 +-- .../PrototypeServiceFactoryUse.java | 2 +- .../serviceregistry/ServiceObjectsImpl.java | 15 +- .../ServiceRegistrationImpl.java | 2 +- .../internal/serviceregistry/ServiceUse.java | 2 +- 11 files changed, 162 insertions(+), 50 deletions(-) create mode 100644 bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/IllegalStateExceptionTests.java diff --git a/bundles/org.eclipse.osgi.tests/META-INF/MANIFEST.MF b/bundles/org.eclipse.osgi.tests/META-INF/MANIFEST.MF index e6f242c9e86..0180cff5d86 100644 --- a/bundles/org.eclipse.osgi.tests/META-INF/MANIFEST.MF +++ b/bundles/org.eclipse.osgi.tests/META-INF/MANIFEST.MF @@ -2,7 +2,7 @@ Manifest-Version: 1.0 Bundle-ManifestVersion: 2 Bundle-Name: Core OSGi Tests Bundle-SymbolicName: org.eclipse.osgi.tests;singleton:=true -Bundle-Version: 3.20.300.qualifier +Bundle-Version: 3.22.100.qualifier Bundle-Vendor: Eclipse.org Require-Bundle: org.eclipse.core.runtime;bundle-version="[3.29.0,4.0.0)", diff --git a/bundles/org.eclipse.osgi.tests/pom.xml b/bundles/org.eclipse.osgi.tests/pom.xml index 74175d32aee..5cd857425a7 100644 --- a/bundles/org.eclipse.osgi.tests/pom.xml +++ b/bundles/org.eclipse.osgi.tests/pom.xml @@ -19,7 +19,7 @@ org.eclipse.osgi org.eclipse.osgi.tests - 3.20.300-SNAPSHOT + 3.22.100-SNAPSHOT eclipse-test-plugin diff --git a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/BundleTests.java b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/BundleTests.java index f0ab86d6a74..7f59115958d 100644 --- a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/BundleTests.java +++ b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/BundleTests.java @@ -40,7 +40,8 @@ PlatformAdminBundleTests.class, // ListenerTests.class, // AddDynamicImportTests.class, // - BundleNativeCodeTests.class // + BundleNativeCodeTests.class, // + IllegalStateExceptionTests.class }) public class BundleTests { } diff --git a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/ExceptionMessageTest.java b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/ExceptionMessageTest.java index f00805d56e4..99d03ddcd44 100644 --- a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/ExceptionMessageTest.java +++ b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/ExceptionMessageTest.java @@ -26,7 +26,6 @@ import org.eclipse.osgi.tests.OSGiTestsActivator; import org.junit.Test; import org.osgi.framework.Bundle; -import org.osgi.framework.BundleContext; import org.osgi.framework.BundleException; import org.osgi.framework.Constants; import org.osgi.framework.FrameworkUtil; @@ -52,16 +51,6 @@ public void testUninstallModuleError() throws BundleException { assertTrue("Wrong message: " + e.getMessage(), e.getMessage().endsWith(b.adapt(Module.class).toString())); } - @Test - public void testUninstallContextError() throws BundleException { - Bundle b = installer.installBundle("test"); - b.start(); - BundleContext context = b.getBundleContext(); - b.uninstall(); - IllegalStateException e = assertThrows(IllegalStateException.class, () -> context.createFilter("(a=b)")); - assertTrue("Wrong message: " + e.getMessage(), e.getMessage().endsWith(b.toString())); - } - @Test public void testStartFragmentError() throws BundleException, IOException { Map headers = new HashMap<>(); @@ -101,23 +90,10 @@ public void testUnregisterSetPropsError() throws BundleException { IllegalStateException e1 = assertThrows(IllegalStateException.class, () -> reg.setProperties(props2)); assertTrue("Wrong message: " + e1.getMessage(), e1.getMessage().endsWith(reg.toString())); - IllegalStateException e2 = assertThrows(IllegalStateException.class, () -> reg.unregister()); - assertTrue("Wrong message: " + e2.getMessage(), e2.getMessage().endsWith(reg.toString())); - IllegalStateException e3 = assertThrows(IllegalStateException.class, () -> reg.getReference()); assertTrue("Wrong message: " + e3.getMessage(), e3.getMessage().endsWith(reg.toString())); } - @Test - public void testUnregisterTwiceError() throws BundleException { - Bundle b = installer.installBundle("test"); - b.start(); - BundleContext context = b.getBundleContext(); - ServiceRegistration reg = context.registerService(Object.class, new Object(), - getDicinotary("k1", "v1")); - reg.unregister(); - } - private Dictionary getDicinotary(String key, Object value) { return FrameworkUtil.asDictionary(Collections.singletonMap(key, value)); } diff --git a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/IllegalStateExceptionTests.java b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/IllegalStateExceptionTests.java new file mode 100644 index 00000000000..b390408d069 --- /dev/null +++ b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/IllegalStateExceptionTests.java @@ -0,0 +1,138 @@ +/******************************************************************************* + * Copyright (c) 2024 IBM Corporation and others. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + * + * Contributors: + * IBM Corporation - initial API and implementation + *******************************************************************************/ +package org.eclipse.osgi.tests.bundles; + +import static org.junit.Assert.assertNotNull; + +import org.junit.Test; +import org.osgi.framework.Bundle; +import org.osgi.framework.BundleContext; +import org.osgi.framework.BundleEvent; +import org.osgi.framework.BundleListener; +import org.osgi.framework.Constants; +import org.osgi.framework.Filter; +import org.osgi.framework.FrameworkEvent; +import org.osgi.framework.FrameworkListener; +import org.osgi.framework.ServiceEvent; +import org.osgi.framework.ServiceListener; +import org.osgi.framework.ServiceObjects; +import org.osgi.framework.ServiceReference; +import org.osgi.framework.ServiceRegistration; +import org.osgi.service.condition.Condition; + +public class IllegalStateExceptionTests extends AbstractBundleTests { + + @Test + public void testUninstall() throws Exception { + Bundle bundle = installer.installBundle("test"); + bundle.uninstall(); + // uninstall again should not throw an exception + bundle.uninstall(); + } + + @Test + public void testCreateFilter() throws Exception { + Filter testFilter = getInvalidContext().createFilter("(test=illegalstateexception)"); + assertNotNull("Null filter returned", testFilter); + } + + @Test + public void testGetBundle() throws Exception { + BundleContext invalidContext = getInvalidContext(); + + assertNotNull("Null bundle returned", invalidContext.getBundle()); + assertNotNull("Null system bundle returned", invalidContext.getBundle(Constants.SYSTEM_BUNDLE_LOCATION)); + assertNotNull("Null system bundle returned", invalidContext.getBundle(Constants.SYSTEM_BUNDLE_ID)); + } + + @Test + public void testGetProperty() throws Exception { + BundleContext invalidContext = getInvalidContext(); + + assertNotNull("Null UUID Property", invalidContext.getProperty(Constants.FRAMEWORK_UUID)); + } + + @Test + public void testRemoveServiceListener() throws Exception { + BundleContext invalidContext = getInvalidContext(); + + invalidContext.removeServiceListener(new ServiceListener() { + @Override + public void serviceChanged(ServiceEvent event) { + // nothing + } + }); + } + + @Test + public void testRemoveBundleListener() throws Exception { + BundleContext invalidContext = getInvalidContext(); + + invalidContext.removeBundleListener(new BundleListener() { + @Override + public void bundleChanged(BundleEvent event) { + // nothing + } + }); + } + + @Test + public void testFrameworkListener() throws Exception { + BundleContext invalidContext = getInvalidContext(); + + invalidContext.removeFrameworkListener(new FrameworkListener() { + @Override + public void frameworkEvent(FrameworkEvent event) { + // nothing + } + }); + } + + @Test + public void testContextUngetService() throws Exception { + BundleContext invalidContext = getInvalidContext(); + + invalidContext.ungetService(getContext().getServiceReference("org.osgi.service.condition.Condition")); + } + + @Test + public void testServiceObjectsUngetService() throws Exception { + Bundle bundle = installer.installBundle("test"); + bundle.start(); + BundleContext context = bundle.getBundleContext(); + ServiceReference ref = context.getServiceReference(Condition.class); + ServiceObjects serviceObjects = context.getServiceObjects(ref); + Condition c = serviceObjects.getService(); + bundle.stop(); + serviceObjects.ungetService(c); + } + + @Test + public void testUnregister() throws Exception { + Bundle bundle = installer.installBundle("test"); + bundle.start(); + BundleContext context = bundle.getBundleContext(); + ServiceRegistration reg = context.registerService(Condition.class, Condition.INSTANCE, null); + reg.unregister(); + reg.unregister(); + } + + public BundleContext getInvalidContext() throws Exception { + Bundle bundle = installer.installBundle("test"); + bundle.start(); + BundleContext context = bundle.getBundleContext(); + bundle.stop(); + return context; + } +} diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleContainer.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleContainer.java index 79a038afdcd..c25faf7fc3a 100644 --- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleContainer.java +++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleContainer.java @@ -616,7 +616,10 @@ public void uninstall(Module module) throws BundleException { module.lockStateChange(ModuleEvent.UNINSTALLED); State previousState; try { - module.checkValid(); + if (module.getState().equals(State.UNINSTALLED)) { + // no longer throwing an IllegalStateException here. + return; + } previousState = module.getState(); if (Module.ACTIVE_SET.contains(module.getState())) { try { diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/framework/BundleContextImpl.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/framework/BundleContextImpl.java index 9bb0028cdd9..0f39a5503c2 100644 --- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/framework/BundleContextImpl.java +++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/framework/BundleContextImpl.java @@ -160,8 +160,6 @@ public String getProperty(String key) { */ @Override public Bundle getBundle() { - checkValid(); - return getBundleImpl(); } @@ -305,12 +303,9 @@ public void addServiceListener(ServiceListener listener) { * method does nothing. * * @param listener The service listener to remove. - * @exception java.lang.IllegalStateException If the bundle context has stopped. */ @Override public void removeServiceListener(ServiceListener listener) { - checkValid(); - if (listener == null) { throw new IllegalArgumentException(); } @@ -354,11 +349,9 @@ public void addBundleListener(BundleListener listener) { * method does nothing. * * @param listener The bundle listener to remove. - * @exception java.lang.IllegalStateException If the bundle context has stopped. */ @Override public void removeBundleListener(BundleListener listener) { - checkValid(); if (listener == null) { throw new IllegalArgumentException(); } @@ -409,11 +402,9 @@ public void addFrameworkListener(FrameworkListener listener) { * method does nothing. * * @param listener The framework listener to remove. - * @exception java.lang.IllegalStateException If the bundle context has stopped. */ @Override public void removeFrameworkListener(FrameworkListener listener) { - checkValid(); if (listener == null) { throw new IllegalArgumentException(); } @@ -708,8 +699,6 @@ public S getService(ServiceReference reference) { */ @Override public boolean ungetService(ServiceReference reference) { - checkValid(); - return container.getServiceRegistry().ungetService(this, (ServiceReferenceImpl) reference); } @@ -1043,13 +1032,9 @@ public void dispatchEvent(Object originalListener, Object l, int action, Object * * @param filter The filter string. * @return A Filter object encapsulating the filter string. - * @exception InvalidSyntaxException If the filter parameter contains an invalid - * filter string which cannot be parsed. */ @Override public Filter createFilter(String filter) throws InvalidSyntaxException { - checkValid(); - return FilterImpl.newInstance(filter, container.getConfiguration().getDebug().DEBUG_FILTER); } @@ -1070,7 +1055,7 @@ public void checkValid() { * * @return true if the context is still valid; false otherwise */ - protected boolean isValid() { + public boolean isValid() { return valid; } diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/serviceregistry/PrototypeServiceFactoryUse.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/serviceregistry/PrototypeServiceFactoryUse.java index 783534658d4..43e8e794120 100644 --- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/serviceregistry/PrototypeServiceFactoryUse.java +++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/serviceregistry/PrototypeServiceFactoryUse.java @@ -96,7 +96,7 @@ S newServiceObject() { @Override boolean releaseServiceObject(final S service) { assert isLocked(); - if ((service == null) || !serviceObjects.containsKey(service)) { + if (((service == null) || !serviceObjects.containsKey(service)) && context.isValid()) { throw new IllegalArgumentException(Msg.SERVICE_OBJECTS_UNGET_ARGUMENT_EXCEPTION); } if (debug.DEBUG_SERVICES) { diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/serviceregistry/ServiceObjectsImpl.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/serviceregistry/ServiceObjectsImpl.java index f71a9aab389..9efd7fec826 100644 --- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/serviceregistry/ServiceObjectsImpl.java +++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/serviceregistry/ServiceObjectsImpl.java @@ -16,7 +16,15 @@ import org.eclipse.osgi.internal.framework.BundleContextImpl; import org.eclipse.osgi.internal.messages.Msg; -import org.osgi.framework.*; +import org.osgi.framework.Bundle; +import org.osgi.framework.BundleContext; +import org.osgi.framework.Constants; +import org.osgi.framework.FrameworkEvent; +import org.osgi.framework.PrototypeServiceFactory; +import org.osgi.framework.ServiceException; +import org.osgi.framework.ServiceObjects; +import org.osgi.framework.ServiceReference; +import org.osgi.framework.ServiceRegistration; /** * ServiceObjects implementation. @@ -132,13 +140,14 @@ public S getService() { */ @Override public void ungetService(S service) { - user.checkValid(); boolean removed = registration.ungetService(user, ServiceConsumer.prototypeConsumer, service); if (!removed) { if (registration.isUnregistered()) { return; } - throw new IllegalArgumentException(Msg.SERVICE_OBJECTS_UNGET_ARGUMENT_EXCEPTION); + if (user.isValid()) { + throw new IllegalArgumentException(Msg.SERVICE_OBJECTS_UNGET_ARGUMENT_EXCEPTION); + } } } 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 f4e6b339a11..252f362c610 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 @@ -225,7 +225,7 @@ public void unregister() { synchronized (registry) { synchronized (registrationLock) { if (state != REGISTERED) { /* in the process of unregisterING */ - throw new IllegalStateException(Msg.SERVICE_ALREADY_UNREGISTERED_EXCEPTION + ' ' + this); + return; } /* remove this object from the service registry */ diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/serviceregistry/ServiceUse.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/serviceregistry/ServiceUse.java index 0a729e19fa4..e27442f925f 100644 --- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/serviceregistry/ServiceUse.java +++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/serviceregistry/ServiceUse.java @@ -149,7 +149,7 @@ S newServiceObject() { */ /* @GuardedBy("getLock()") */ boolean releaseServiceObject(final S service) { - if ((service == null) || (service != getCachedService())) { + if ((service == null) || (service != getCachedService()) && context.isValid()) { throw new IllegalArgumentException(Msg.SERVICE_OBJECTS_UNGET_ARGUMENT_EXCEPTION); } if (debug.DEBUG_SERVICES) {