From 41e61e6e57b12baa547ace1a04dd0c066b93526f Mon Sep 17 00:00:00 2001 From: Richard Eckart de Castilho Date: Wed, 18 Dec 2024 14:14:29 +0100 Subject: [PATCH 1/6] Issue #431: Issue using SPI-enabled type systems embedded into PEARs - Try preventing the loading of UIMA SPI providers via the PEAR mechanism --- .../uima/internal/util/UIMAClassLoader.java | 47 +++++++++++++------ 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/uimaj-core/src/main/java/org/apache/uima/internal/util/UIMAClassLoader.java b/uimaj-core/src/main/java/org/apache/uima/internal/util/UIMAClassLoader.java index 6729327ba..35d7877e7 100644 --- a/uimaj-core/src/main/java/org/apache/uima/internal/util/UIMAClassLoader.java +++ b/uimaj-core/src/main/java/org/apache/uima/internal/util/UIMAClassLoader.java @@ -31,6 +31,11 @@ import org.apache.uima.UIMAFramework; import org.apache.uima.cas.impl.FSClassRegistry; import org.apache.uima.cas.impl.TypeSystemImpl; +import org.apache.uima.spi.FsIndexCollectionProvider; +import org.apache.uima.spi.JCasClassProvider; +import org.apache.uima.spi.TypePrioritiesProvider; +import org.apache.uima.spi.TypeSystemDescriptionProvider; +import org.apache.uima.spi.TypeSystemProvider; /** * UIMAClassLoader is used as extension ClassLoader for UIMA to load additional components like @@ -234,11 +239,16 @@ protected Class loadClass(String name, boolean resolve) throws ClassNotFoundE // class loader and class name pair. // pick a random syncLock to synchronize // Although the sync locks are not one/per/class, there should be enough of them to make the - // likelyhood - // of needing to wait very low (unless it's the same class-name being loaded, of course). + // likelihood of needing to wait very low (unless it's the same class-name being loaded, of + // course). synchronized (syncLocks[name.hashCode() & (nbrLocks - 1)]) { - // First, check if the class has already been loaded - Class c = findLoadedClass(name); + Class c = null; + + if (c == null) { + // Check if the class has already been loaded + c = findLoadedClass(name); + } + if (c == null) { try { // try to load class @@ -262,6 +272,19 @@ protected Class loadClass(String name, boolean resolve) throws ClassNotFoundE if (resolve) { resolveClass(c); + + // Accessing the interfaces would implicitly trigger resolution - so we can only do it when + // resolution is + // allowed... hopefully nobody tries to load SPIs without also allowing to resolve the class + if (TypeSystemProvider.class.isAssignableFrom(c) + || TypeSystemDescriptionProvider.class.isAssignableFrom(c) + || JCasClassProvider.class.isAssignableFrom(c) + || FsIndexCollectionProvider.class.isAssignableFrom(c) + || TypePrioritiesProvider.class.isAssignableFrom(c)) { + // We never want to return local SPI implementations - + // https://github.com/apache/uima-uimaj/issues/431 + c = super.loadClass(name, false); + } } return c; @@ -273,15 +296,16 @@ private boolean isUimaInternalPackage(String name) { || name.startsWith("org.apache.uima.jcas.cas."); } - /* - * loads resource from this class loader first, if possible (non-Javadoc) - * - * @see java.lang.ClassLoader#getResource(java.lang.String) - */ @Override public URL getResource(String name) { synchronized (syncLocks[name.hashCode() & (nbrLocks - 1)]) { // https://issues.apache.org/jira/browse/UIMA-5741 + if (name != null && name.contains("META-INF/services/org.apache.uima.spi.")) { + // We never want to return local SPI implementations + // https://github.com/apache/uima-uimaj/issues/431 + return super.getResource(name); + } + URL url = findResource(name); if (null == url) { @@ -303,11 +327,6 @@ public boolean isClosed() { return isClosed; } - /* - * (non-Javadoc) - * - * @see java.net.URLClassLoader#close() - */ @Override public void close() throws IOException { isClosed = true; From 4782d812364867da6f990dc8cf98e6637f4470bc Mon Sep 17 00:00:00 2001 From: Richard Eckart de Castilho Date: Wed, 18 Dec 2024 17:03:27 +0100 Subject: [PATCH 2/6] Issue #431: Issue using SPI-enabled type systems embedded into PEARs - We have to always look at the interfaces - because the method will typically be called with resolve=false and then the SPIs slip through --- .../uima/internal/util/UIMAClassLoader.java | 23 +++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/uimaj-core/src/main/java/org/apache/uima/internal/util/UIMAClassLoader.java b/uimaj-core/src/main/java/org/apache/uima/internal/util/UIMAClassLoader.java index 35d7877e7..b5d038393 100644 --- a/uimaj-core/src/main/java/org/apache/uima/internal/util/UIMAClassLoader.java +++ b/uimaj-core/src/main/java/org/apache/uima/internal/util/UIMAClassLoader.java @@ -272,19 +272,18 @@ protected Class loadClass(String name, boolean resolve) throws ClassNotFoundE if (resolve) { resolveClass(c); + } - // Accessing the interfaces would implicitly trigger resolution - so we can only do it when - // resolution is - // allowed... hopefully nobody tries to load SPIs without also allowing to resolve the class - if (TypeSystemProvider.class.isAssignableFrom(c) - || TypeSystemDescriptionProvider.class.isAssignableFrom(c) - || JCasClassProvider.class.isAssignableFrom(c) - || FsIndexCollectionProvider.class.isAssignableFrom(c) - || TypePrioritiesProvider.class.isAssignableFrom(c)) { - // We never want to return local SPI implementations - - // https://github.com/apache/uima-uimaj/issues/431 - c = super.loadClass(name, false); - } + // Accessing the interfaces will implicitly trigger resolution - but we can't really help + // ourselves at the moment + if (TypeSystemProvider.class.isAssignableFrom(c) + || TypeSystemDescriptionProvider.class.isAssignableFrom(c) + || JCasClassProvider.class.isAssignableFrom(c) + || FsIndexCollectionProvider.class.isAssignableFrom(c) + || TypePrioritiesProvider.class.isAssignableFrom(c)) { + // We never want to return local SPI implementations - + // https://github.com/apache/uima-uimaj/issues/431 + c = super.loadClass(name, false); } return c; From 25043f0b3b2238ee1e7738e3c1fba608d31cb255 Mon Sep 17 00:00:00 2001 From: Richard Eckart de Castilho Date: Wed, 18 Dec 2024 17:37:47 +0100 Subject: [PATCH 3/6] Issue #431: Issue using SPI-enabled type systems embedded into PEARs - Invoke delegate classloader directly to avoid that super.loadClass() finds the wrong cached class via findLoadedClass --- .../org/apache/uima/internal/util/UIMAClassLoader.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/uimaj-core/src/main/java/org/apache/uima/internal/util/UIMAClassLoader.java b/uimaj-core/src/main/java/org/apache/uima/internal/util/UIMAClassLoader.java index b5d038393..3880cdd7c 100644 --- a/uimaj-core/src/main/java/org/apache/uima/internal/util/UIMAClassLoader.java +++ b/uimaj-core/src/main/java/org/apache/uima/internal/util/UIMAClassLoader.java @@ -281,9 +281,15 @@ protected Class loadClass(String name, boolean resolve) throws ClassNotFoundE || JCasClassProvider.class.isAssignableFrom(c) || FsIndexCollectionProvider.class.isAssignableFrom(c) || TypePrioritiesProvider.class.isAssignableFrom(c)) { - // We never want to return local SPI implementations - + + // We never want to return local SPI implementations loaded by the UIMAClassLoader // https://github.com/apache/uima-uimaj/issues/431 - c = super.loadClass(name, false); + var parent = getParent(); + if (parent != null) { + c = parent.loadClass(name); + } else { + c = ClassLoader.getSystemClassLoader().loadClass(name); + } } return c; From 74f9b802dca023f7c08447d79f8fda3fe27fbc37 Mon Sep 17 00:00:00 2001 From: Richard Eckart de Castilho Date: Wed, 18 Dec 2024 18:15:58 +0100 Subject: [PATCH 4/6] Issue #431: Issue using SPI-enabled type systems embedded into PEARs - When dealing with UIMA SPIs in getResource() also delegate directly --- .../uima/internal/util/UIMAClassLoader.java | 59 ++++++++++++++++++- 1 file changed, 57 insertions(+), 2 deletions(-) diff --git a/uimaj-core/src/main/java/org/apache/uima/internal/util/UIMAClassLoader.java b/uimaj-core/src/main/java/org/apache/uima/internal/util/UIMAClassLoader.java index 3880cdd7c..c0ef4cce5 100644 --- a/uimaj-core/src/main/java/org/apache/uima/internal/util/UIMAClassLoader.java +++ b/uimaj-core/src/main/java/org/apache/uima/internal/util/UIMAClassLoader.java @@ -19,13 +19,17 @@ package org.apache.uima.internal.util; +import static java.util.Collections.emptyEnumeration; + import java.io.File; import java.io.IOException; import java.net.MalformedURLException; import java.net.URL; import java.net.URLClassLoader; import java.util.ArrayList; +import java.util.Enumeration; import java.util.List; +import java.util.NoSuchElementException; import java.util.StringTokenizer; import org.apache.uima.UIMAFramework; @@ -301,14 +305,39 @@ private boolean isUimaInternalPackage(String name) { || name.startsWith("org.apache.uima.jcas.cas."); } + @Override + public Enumeration getResources(String name) throws IOException { + synchronized (syncLocks[name.hashCode() & (nbrLocks - 1)]) { // https://issues.apache.org/jira/browse/UIMA-5741 + Enumeration delegateResources; + var parent = getParent(); + if (parent != null) { + delegateResources = parent.getResources(name); + } else { + delegateResources = ClassLoader.getSystemClassLoader().getResources(name); + } + + Enumeration localResources = emptyEnumeration(); + if (!name.contains("META-INF/services/org.apache.uima.spi.")) { + localResources = findResources(name); + } + + return new CombinedEnumeration<>(localResources, delegateResources); + } + } + @Override public URL getResource(String name) { synchronized (syncLocks[name.hashCode() & (nbrLocks - 1)]) { // https://issues.apache.org/jira/browse/UIMA-5741 - if (name != null && name.contains("META-INF/services/org.apache.uima.spi.")) { + if (name.contains("META-INF/services/org.apache.uima.spi.")) { // We never want to return local SPI implementations // https://github.com/apache/uima-uimaj/issues/431 - return super.getResource(name); + var parent = getParent(); + if (parent != null) { + return parent.getResource(name); + } else { + return ClassLoader.getSystemClassLoader().getResource(name); + } } URL url = findResource(name); @@ -352,4 +381,30 @@ public void close() throws IOException { Object getClassLoadingLockForTesting(String aClassName) { return super.getClassLoadingLock(aClassName); } + + static class CombinedEnumeration implements Enumeration { + private final Enumeration first; + private final Enumeration second; + + public CombinedEnumeration(Enumeration first, Enumeration second) { + this.first = first; + this.second = second; + } + + @Override + public boolean hasMoreElements() { + return first.hasMoreElements() || second.hasMoreElements(); + } + + @Override + public T nextElement() { + if (first.hasMoreElements()) { + return first.nextElement(); + } else if (second.hasMoreElements()) { + return second.nextElement(); + } else { + throw new NoSuchElementException("No more elements"); + } + } + } } From 6bc4124410a75822fa352ad493023aed55d14221 Mon Sep 17 00:00:00 2001 From: Richard Eckart de Castilho Date: Thu, 19 Dec 2024 11:44:26 +0100 Subject: [PATCH 5/6] Issue #431: Issue using SPI-enabled type systems embedded into PEARs - Cleaning up a a bit - Added test - Added service loading wrapper that logs if a service cannot be loaded but does not immediately crash the system (unless there are too many broken services) --- .../uima/fit/factory/FsIndexFactory.java | 6 +- .../fit/factory/TypePrioritiesFactory.java | 6 +- .../factory/TypeSystemDescriptionFactory.java | 6 +- .../apache/uima/fit/validation/Validator.java | 5 +- .../apache/uima/cas/impl/FSClassRegistry.java | 6 +- .../uima/internal/util/ServiceLoaderUtil.java | 112 ++++++++++++++++++ .../uima/internal/util/UIMAClassLoader.java | 42 ++++--- .../resource/metadata/impl/Import_impl.java | 18 +-- .../internal/util/ServiceLoaderUtilTest.java | 69 +++++++++++ .../internal/util/UIMAClassLoaderTest.java | 64 ++++++---- ...IMAClassLoaderTest_TypeSystemProvider.java | 36 ++++++ .../ClassLoaderTest/classLoadingTest.jar | Bin 659 -> 1810 bytes 12 files changed, 308 insertions(+), 62 deletions(-) create mode 100644 uimaj-core/src/main/java/org/apache/uima/internal/util/ServiceLoaderUtil.java create mode 100644 uimaj-core/src/test/java/org/apache/uima/internal/util/ServiceLoaderUtilTest.java create mode 100644 uimaj-core/src/test/java/org/apache/uima/internal/util/UIMAClassLoaderTest_TypeSystemProvider.java diff --git a/uimafit-core/src/main/java/org/apache/uima/fit/factory/FsIndexFactory.java b/uimafit-core/src/main/java/org/apache/uima/fit/factory/FsIndexFactory.java index 902403b0b..2c6e63315 100644 --- a/uimafit-core/src/main/java/org/apache/uima/fit/factory/FsIndexFactory.java +++ b/uimafit-core/src/main/java/org/apache/uima/fit/factory/FsIndexFactory.java @@ -22,6 +22,7 @@ import static org.apache.uima.UIMAFramework.getXMLParser; import static org.apache.uima.fit.internal.MetaDataUtil.scanDescriptors; import static org.apache.uima.fit.internal.ReflectionUtil.getInheritableAnnotation; +import static org.apache.uima.internal.util.ServiceLoaderUtil.loadServicesSafely; import java.io.IOException; import java.util.ArrayList; @@ -31,7 +32,6 @@ import java.util.IdentityHashMap; import java.util.List; import java.util.Map; -import java.util.ServiceLoader; import java.util.WeakHashMap; import org.apache.uima.fit.descriptor.FsIndex; @@ -335,7 +335,7 @@ static void loadFsIndexCollectionsFromScannedLocations(List static void loadFsIndexCollectionsfromSPIs(List fsIndexList) { var loaded = Collections.newSetFromMap(new IdentityHashMap<>()); - ServiceLoader.load(FsIndexCollectionProvider.class).forEach(provider -> { + loadServicesSafely(FsIndexCollectionProvider.class).forEach(provider -> { for (var fsIdxCol : provider.listFsIndexCollections()) { loaded.add(fsIdxCol); fsIndexList.addAll(asList(fsIdxCol.getFsIndexes())); @@ -344,7 +344,7 @@ static void loadFsIndexCollectionsfromSPIs(List fsIndexList) } }); - ServiceLoader.load(TypeSystemProvider.class).forEach(provider -> { + loadServicesSafely(TypeSystemProvider.class).forEach(provider -> { for (var fsIdxCol : provider.listFsIndexCollections()) { if (loaded.contains(fsIdxCol)) { continue; diff --git a/uimafit-core/src/main/java/org/apache/uima/fit/factory/TypePrioritiesFactory.java b/uimafit-core/src/main/java/org/apache/uima/fit/factory/TypePrioritiesFactory.java index 95ebb2c52..cb54e9555 100644 --- a/uimafit-core/src/main/java/org/apache/uima/fit/factory/TypePrioritiesFactory.java +++ b/uimafit-core/src/main/java/org/apache/uima/fit/factory/TypePrioritiesFactory.java @@ -21,6 +21,7 @@ import static org.apache.uima.UIMAFramework.getXMLParser; import static org.apache.uima.fit.internal.MetaDataUtil.scanDescriptors; import static org.apache.uima.fit.util.CasUtil.UIMA_BUILTIN_JCAS_PREFIX; +import static org.apache.uima.internal.util.ServiceLoaderUtil.loadServicesSafely; import java.io.IOException; import java.util.ArrayList; @@ -29,7 +30,6 @@ import java.util.IdentityHashMap; import java.util.List; import java.util.Map; -import java.util.ServiceLoader; import java.util.WeakHashMap; import org.apache.uima.fit.internal.ClassLoaderUtils; @@ -170,7 +170,7 @@ static void loadTypePrioritiesFromScannedLocations(List typePrio static void loadTypePrioritiesFromSPIs(List typePrioritiesList) { var loaded = Collections.newSetFromMap(new IdentityHashMap<>()); - ServiceLoader.load(TypePrioritiesProvider.class).forEach(provider -> { + loadServicesSafely(TypePrioritiesProvider.class).forEach(provider -> { for (var desc : provider.listTypePriorities()) { loaded.add(desc); typePrioritiesList.add(desc); @@ -178,7 +178,7 @@ static void loadTypePrioritiesFromSPIs(List typePrioritiesList) } }); - ServiceLoader.load(TypeSystemProvider.class).forEach(provider -> { + loadServicesSafely(TypeSystemProvider.class).forEach(provider -> { for (var desc : provider.listTypePriorities()) { if (loaded.contains(desc)) { continue; diff --git a/uimafit-core/src/main/java/org/apache/uima/fit/factory/TypeSystemDescriptionFactory.java b/uimafit-core/src/main/java/org/apache/uima/fit/factory/TypeSystemDescriptionFactory.java index d8ad46bc5..879eadfec 100644 --- a/uimafit-core/src/main/java/org/apache/uima/fit/factory/TypeSystemDescriptionFactory.java +++ b/uimafit-core/src/main/java/org/apache/uima/fit/factory/TypeSystemDescriptionFactory.java @@ -20,6 +20,7 @@ import static org.apache.uima.UIMAFramework.getXMLParser; import static org.apache.uima.fit.internal.MetaDataUtil.scanDescriptors; +import static org.apache.uima.internal.util.ServiceLoaderUtil.loadServicesSafely; import static org.apache.uima.util.CasCreationUtils.mergeTypeSystems; import java.io.IOException; @@ -28,7 +29,6 @@ import java.util.HashMap; import java.util.IdentityHashMap; import java.util.List; -import java.util.ServiceLoader; import java.util.WeakHashMap; import org.apache.uima.fit.internal.ClassLoaderUtils; @@ -173,7 +173,7 @@ static void loadTypeSystemDescriptionsFromScannedLocations(List tsdList) { var loaded = Collections.newSetFromMap(new IdentityHashMap<>()); - ServiceLoader.load(TypeSystemDescriptionProvider.class).forEach(provider -> { + loadServicesSafely(TypeSystemDescriptionProvider.class).forEach(provider -> { for (var desc : provider.listTypeSystemDescriptions()) { loaded.add(desc); tsdList.add(desc); @@ -181,7 +181,7 @@ static void loadTypeSystemDescriptionsFromSPIs(List tsdLi } }); - ServiceLoader.load(TypeSystemProvider.class).forEach(provider -> { + loadServicesSafely(TypeSystemProvider.class).forEach(provider -> { for (var desc : provider.listTypeSystemDescriptions()) { if (loaded.contains(desc)) { continue; diff --git a/uimafit-core/src/main/java/org/apache/uima/fit/validation/Validator.java b/uimafit-core/src/main/java/org/apache/uima/fit/validation/Validator.java index f9d8fe0a7..7889b0a6e 100644 --- a/uimafit-core/src/main/java/org/apache/uima/fit/validation/Validator.java +++ b/uimafit-core/src/main/java/org/apache/uima/fit/validation/Validator.java @@ -19,8 +19,7 @@ package org.apache.uima.fit.validation; import static java.util.Arrays.stream; -import static java.util.ServiceLoader.load; -import static java.util.stream.StreamSupport.stream; +import static org.apache.uima.internal.util.ServiceLoaderUtil.loadServicesSafely; import java.util.Collection; import java.util.HashSet; @@ -219,7 +218,7 @@ public Validator.Builder includingByType(Class... types) { } private Validator.Builder autoDetectChecks() { - stream(load(ValidationCheck.class).spliterator(), false).forEachOrdered(checks::add); + loadServicesSafely(ValidationCheck.class).forEachOrdered(checks::add); return this; } diff --git a/uimaj-core/src/main/java/org/apache/uima/cas/impl/FSClassRegistry.java b/uimaj-core/src/main/java/org/apache/uima/cas/impl/FSClassRegistry.java index aad9103b8..8ea3720d2 100644 --- a/uimaj-core/src/main/java/org/apache/uima/cas/impl/FSClassRegistry.java +++ b/uimaj-core/src/main/java/org/apache/uima/cas/impl/FSClassRegistry.java @@ -22,6 +22,7 @@ import static java.lang.invoke.MethodHandles.lookup; import static java.lang.invoke.MethodType.methodType; import static java.util.Collections.emptyMap; +import static org.apache.uima.internal.util.ServiceLoaderUtil.loadServicesSafely; import java.lang.invoke.LambdaMetafactory; import java.lang.invoke.MethodHandle; @@ -40,7 +41,6 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; -import java.util.ServiceLoader; import org.apache.uima.UIMAFramework; import org.apache.uima.UIMARuntimeException; @@ -1009,14 +1009,14 @@ static Map> loadJCasClassesFromSPI(ClassLoader cl) var spiJCasClasses = new LinkedHashMap>(); - ServiceLoader.load(JCasClassProvider.class, cl).forEach(provider -> { + loadServicesSafely(JCasClassProvider.class, cl).forEach(provider -> { var list = provider.listJCasClasses(); if (list != null) { list.forEach(item -> spiJCasClasses.put(item.getName(), item)); } }); - ServiceLoader.load(TypeSystemProvider.class, cl).forEach(provider -> { + loadServicesSafely(TypeSystemProvider.class, cl).forEach(provider -> { var list = provider.listJCasClasses(); if (list != null) { list.forEach(item -> spiJCasClasses.put(item.getName(), item)); diff --git a/uimaj-core/src/main/java/org/apache/uima/internal/util/ServiceLoaderUtil.java b/uimaj-core/src/main/java/org/apache/uima/internal/util/ServiceLoaderUtil.java new file mode 100644 index 000000000..7ffceaef0 --- /dev/null +++ b/uimaj-core/src/main/java/org/apache/uima/internal/util/ServiceLoaderUtil.java @@ -0,0 +1,112 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.uima.internal.util; + +import java.lang.invoke.MethodHandles; +import java.util.Collection; +import java.util.Iterator; +import java.util.ServiceConfigurationError; +import java.util.ServiceLoader; +import java.util.Spliterator; +import java.util.function.Consumer; +import java.util.stream.Stream; +import java.util.stream.StreamSupport; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class ServiceLoaderUtil { + private static final Logger LOG = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); + private static final int MAX_BROKEN_SERVICES = 16; + + public static Stream loadServicesSafely(Class aService) { + var cl = ClassLoaderUtils.findClassLoader(); + return loadServicesSafely(aService, cl, null); + } + + public static Stream loadServicesSafely(Class aService, ClassLoader aClassLoader) { + return loadServicesSafely(aService, aClassLoader, null); + } + + static Stream loadServicesSafely(Class aService, ClassLoader aClassLoader, + Collection aErrorCollector) { + var loader = ServiceLoader.load(aService, aClassLoader); + return StreamSupport.stream( + new ServiceLoaderSpliterator(aService, loader.iterator(), aErrorCollector), false); + } + + private static class ServiceLoaderSpliterator implements Spliterator { + + private final Logger log; + private final Iterator serviceIterator; + private final Class service; + private final Collection errorCollector; + + public ServiceLoaderSpliterator(Class aService, Iterator aIterator, + Collection aErrorCollector) { + serviceIterator = aIterator; + service = aService; + errorCollector = aErrorCollector; + log = aErrorCollector == null ? LOG : null; + } + + @Override + public boolean tryAdvance(final Consumer aAction) { + int i = MAX_BROKEN_SERVICES; + while (i-- > 0) { + try { + if (serviceIterator.hasNext()) { + aAction.accept(serviceIterator.next()); + return true; + } + } catch (ServiceConfigurationError | LinkageError e) { + handle(e); + } catch (Throwable e) { + handle(e); + throw e; + } + } + return false; + } + + private void handle(Throwable e) { + if (log != null) { + log.warn("Unable to load service class for service {}", service, e); + } + if (errorCollector != null) { + errorCollector.add(e); + } + } + + @Override + public Spliterator trySplit() { + return null; + } + + @Override + public long estimateSize() { + return Long.MAX_VALUE; + } + + @Override + public int characteristics() { + return NONNULL | IMMUTABLE; + } + } +} diff --git a/uimaj-core/src/main/java/org/apache/uima/internal/util/UIMAClassLoader.java b/uimaj-core/src/main/java/org/apache/uima/internal/util/UIMAClassLoader.java index c0ef4cce5..ab37acc90 100644 --- a/uimaj-core/src/main/java/org/apache/uima/internal/util/UIMAClassLoader.java +++ b/uimaj-core/src/main/java/org/apache/uima/internal/util/UIMAClassLoader.java @@ -280,12 +280,7 @@ protected Class loadClass(String name, boolean resolve) throws ClassNotFoundE // Accessing the interfaces will implicitly trigger resolution - but we can't really help // ourselves at the moment - if (TypeSystemProvider.class.isAssignableFrom(c) - || TypeSystemDescriptionProvider.class.isAssignableFrom(c) - || JCasClassProvider.class.isAssignableFrom(c) - || FsIndexCollectionProvider.class.isAssignableFrom(c) - || TypePrioritiesProvider.class.isAssignableFrom(c)) { - + if (isUimaSpiImplementation(c)) { // We never want to return local SPI implementations loaded by the UIMAClassLoader // https://github.com/apache/uima-uimaj/issues/431 var parent = getParent(); @@ -300,11 +295,6 @@ protected Class loadClass(String name, boolean resolve) throws ClassNotFoundE } } - private boolean isUimaInternalPackage(String name) { - return name.startsWith("org.apache.uima.cas.impl.") - || name.startsWith("org.apache.uima.jcas.cas."); - } - @Override public Enumeration getResources(String name) throws IOException { synchronized (syncLocks[name.hashCode() & (nbrLocks - 1)]) { // https://issues.apache.org/jira/browse/UIMA-5741 @@ -317,7 +307,7 @@ public Enumeration getResources(String name) throws IOException { } Enumeration localResources = emptyEnumeration(); - if (!name.contains("META-INF/services/org.apache.uima.spi.")) { + if (!isUimaSpiConfigurationFile(name)) { localResources = findResources(name); } @@ -329,26 +319,44 @@ public Enumeration getResources(String name) throws IOException { public URL getResource(String name) { synchronized (syncLocks[name.hashCode() & (nbrLocks - 1)]) { // https://issues.apache.org/jira/browse/UIMA-5741 - if (name.contains("META-INF/services/org.apache.uima.spi.")) { + if (isUimaSpiConfigurationFile(name)) { // We never want to return local SPI implementations // https://github.com/apache/uima-uimaj/issues/431 var parent = getParent(); if (parent != null) { return parent.getResource(name); - } else { - return ClassLoader.getSystemClassLoader().getResource(name); } + + return ClassLoader.getSystemClassLoader().getResource(name); } - URL url = findResource(name); + var url = findResource(name); if (null == url) { url = super.getResource(name); } + return url; } } + private boolean isUimaSpiImplementation(Class c) { + return TypeSystemProvider.class.isAssignableFrom(c) + || TypeSystemDescriptionProvider.class.isAssignableFrom(c) + || JCasClassProvider.class.isAssignableFrom(c) + || FsIndexCollectionProvider.class.isAssignableFrom(c) + || TypePrioritiesProvider.class.isAssignableFrom(c); + } + + private boolean isUimaInternalPackage(String name) { + return name.startsWith("org.apache.uima.cas.impl.") + || name.startsWith("org.apache.uima.jcas.cas."); + } + + private boolean isUimaSpiConfigurationFile(String name) { + return name.contains("META-INF/services/org.apache.uima.spi."); + } + /** * The UIMA Class Loader extends URLClassLoader. This kind of classloader supports the close() * method. @@ -382,7 +390,7 @@ Object getClassLoadingLockForTesting(String aClassName) { return super.getClassLoadingLock(aClassName); } - static class CombinedEnumeration implements Enumeration { + private static class CombinedEnumeration implements Enumeration { private final Enumeration first; private final Enumeration second; diff --git a/uimaj-core/src/main/java/org/apache/uima/resource/metadata/impl/Import_impl.java b/uimaj-core/src/main/java/org/apache/uima/resource/metadata/impl/Import_impl.java index e678d1853..a2b6fcfa5 100644 --- a/uimaj-core/src/main/java/org/apache/uima/resource/metadata/impl/Import_impl.java +++ b/uimaj-core/src/main/java/org/apache/uima/resource/metadata/impl/Import_impl.java @@ -19,9 +19,11 @@ package org.apache.uima.resource.metadata.impl; +import static org.apache.uima.internal.util.ServiceLoaderUtil.loadServicesSafely; + import java.net.MalformedURLException; import java.net.URL; -import java.util.ServiceLoader; +import java.util.Optional; import org.apache.uima.UIMAFramework; import org.apache.uima.internal.util.ClassLoaderUtils; @@ -132,14 +134,12 @@ private URL findResouceUrlByName(ResourceManager aResourceManager, String name) // If that fails, try loading through the SPIs if (url == null) { var cl = ClassLoaderUtils.findClassLoader(aResourceManager); - var providers = ServiceLoader.load(TypeSystemProvider.class, cl); - for (var provider : providers) { - var maybeTypeSystemUrl = provider.findResourceUrl(name); - if (maybeTypeSystemUrl.isPresent()) { - url = maybeTypeSystemUrl.get(); - break; - } - } + loadServicesSafely(TypeSystemProvider.class, cl) // + .map(provider -> provider.findResourceUrl(name)) // + .filter(Optional::isPresent) // + .map(Optional::get) // + .findFirst() // + .orElse(null); } if (url == null) { diff --git a/uimaj-core/src/test/java/org/apache/uima/internal/util/ServiceLoaderUtilTest.java b/uimaj-core/src/test/java/org/apache/uima/internal/util/ServiceLoaderUtilTest.java new file mode 100644 index 000000000..968e06567 --- /dev/null +++ b/uimaj-core/src/test/java/org/apache/uima/internal/util/ServiceLoaderUtilTest.java @@ -0,0 +1,69 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.uima.internal.util; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.apache.commons.io.FileUtils.writeStringToFile; +import static org.apache.uima.internal.util.ServiceLoaderUtil.loadServicesSafely; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.tuple; + +import java.io.File; +import java.net.URL; +import java.net.URLClassLoader; +import java.util.ArrayList; + +import org.apache.uima.spi.TypeSystemProvider; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +class ServiceLoaderUtilTest { + + @Test + void testLoadingNonExistingService(@TempDir File aTemp) throws Exception { + writeStringToFile(new File(aTemp, "META-INF/services/" + TypeSystemProvider.class.getName()), + "ClassThatDoesNotExist", UTF_8); + + var cl = new URLClassLoader(new URL[] { aTemp.toURL() }); + + var errors = new ArrayList(); + var services = new ArrayList(); + loadServicesSafely(TypeSystemProvider.class, cl, errors).forEach(services::add); + assertThat(services).isEmpty(); + assertThat(errors).hasSize(1); + } + + @Test + void testLoadingService(@TempDir File aTemp) throws Exception { + writeStringToFile(new File(aTemp, "META-INF/services/" + TypeSystemProvider.class.getName()), + UIMAClassLoaderTest_TypeSystemProvider.class.getName(), UTF_8); + + var cl = new URLClassLoader(new URL[] { aTemp.toURL() }, getClass().getClassLoader()); + + var errors = new ArrayList(); + var services = new ArrayList(); + loadServicesSafely(TypeSystemProvider.class, cl, errors).forEach(services::add); + assertThat(services) // + .hasSize(1) // + .extracting(t -> t.getClass(), t -> t.getClass().getClassLoader()) // + .containsExactly(tuple(UIMAClassLoaderTest_TypeSystemProvider.class, + getClass().getClassLoader())); + assertThat(errors).isEmpty(); + } +} diff --git a/uimaj-core/src/test/java/org/apache/uima/internal/util/UIMAClassLoaderTest.java b/uimaj-core/src/test/java/org/apache/uima/internal/util/UIMAClassLoaderTest.java index 58f064faf..d1c15f83d 100644 --- a/uimaj-core/src/test/java/org/apache/uima/internal/util/UIMAClassLoaderTest.java +++ b/uimaj-core/src/test/java/org/apache/uima/internal/util/UIMAClassLoaderTest.java @@ -19,13 +19,15 @@ package org.apache.uima.internal.util; +import static java.util.Arrays.asList; +import static java.util.Collections.list; import static org.assertj.core.api.Assertions.assertThat; import java.io.File; import java.net.URL; +import java.util.zip.ZipFile; import org.apache.uima.UIMAFramework; -import org.apache.uima.resource.ResourceManager; import org.apache.uima.test.junit_extension.JUnitExtension; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -43,7 +45,7 @@ public void setUp() throws Exception { @Test void testSimpleRsrcMgrCLassLoaderCreation() throws Exception { - ResourceManager rsrcMgr = UIMAFramework.newDefaultResourceManager(); + var rsrcMgr = UIMAFramework.newDefaultResourceManager(); assertThat(rsrcMgr.getExtensionClassLoader()) // .as("Freshly created resource manager has no extension classloader") // @@ -55,31 +57,29 @@ void testSimpleRsrcMgrCLassLoaderCreation() throws Exception { .as("After setting an extension classpath, there is an extension classloader") // .isInstanceOf(UIMAClassLoader.class); - UIMAClassLoader cl = (UIMAClassLoader) rsrcMgr.getExtensionClassLoader(); - Object o = cl.getClassLoadingLockForTesting("someString"); - Object o2 = cl.getClassLoadingLockForTesting("s2"); + var cl = (UIMAClassLoader) rsrcMgr.getExtensionClassLoader(); + var o = cl.getClassLoadingLockForTesting("someString"); + var o2 = cl.getClassLoadingLockForTesting("s2"); assertThat(o != o2).isTrue(); assertThat(cl != o).isTrue(); } @Test void testAdvancedRsrcMgrCLassLoaderCreation() throws Exception { - ResourceManager rsrcMgr = UIMAFramework.newDefaultResourceManager(); + var rsrcMgr = UIMAFramework.newDefaultResourceManager(); assertThat(rsrcMgr.getExtensionClassLoader()).isNull(); rsrcMgr.setExtensionClassPath("../this/is/a/simple/test.jar", true); assertThat(rsrcMgr.getExtensionClassLoader()).isNotNull(); - } @Test void testSimpleClassloadingSampleString() throws Exception { - UIMAClassLoader cl = new UIMAClassLoader(testClassPath, this.getClass().getClassLoader()); - Class testClass = null; + var cl = new UIMAClassLoader(testClassPath, this.getClass().getClassLoader()); - testClass = cl.loadClass("org.apache.uima.internal.util.ClassloadingTestClass"); + var testClass = cl.loadClass("org.apache.uima.internal.util.ClassloadingTestClass"); assertThat(testClass).isNotNull(); assertThat(testClass.getClassLoader()).isEqualTo(cl); @@ -105,7 +105,7 @@ public void call(int threadNumber, int repeatNumber, StringBuilder sb) throws Ex MultiThreadUtils.tstMultiThread("MultiThreadLoading", Misc.numberOfCores, 1, callable, MultiThreadUtils.emptyReset); - Class c = loadedClasses[0]; + var c = loadedClasses[0]; for (int i = 1; i < Misc.numberOfCores; i++) { assertThat(loadedClasses[i]).isEqualTo(c); } @@ -113,11 +113,10 @@ public void call(int threadNumber, int repeatNumber, StringBuilder sb) throws Ex @Test void testSimpleClassloadingSampleURL() throws Exception { - URL[] urlClasspath = new URL[] { new File(testClassPath).toURL() }; + var urlClasspath = new URL[] { new File(testClassPath).toURL() }; UIMAClassLoader cl = new UIMAClassLoader(urlClasspath, this.getClass().getClassLoader()); - Class testClass = null; - testClass = cl.loadClass("org.apache.uima.internal.util.ClassloadingTestClass"); + var testClass = cl.loadClass("org.apache.uima.internal.util.ClassloadingTestClass"); assertThat(testClass).isNotNull(); assertThat(testClass.getClassLoader()).isEqualTo(cl); @@ -130,10 +129,9 @@ void testSimpleClassloadingSampleURL() throws Exception { @Test void testAdvancedClassloadingSampleString() throws Exception { - UIMAClassLoader cl = new UIMAClassLoader(testClassPath, this.getClass().getClassLoader()); - Class testClass = null; + var cl = new UIMAClassLoader(testClassPath, this.getClass().getClassLoader()); - testClass = cl.loadClass("org.apache.uima.internal.util.ClassloadingTestClass"); + var testClass = cl.loadClass("org.apache.uima.internal.util.ClassloadingTestClass"); assertThat(testClass).isNotNull(); assertThat(testClass.getClassLoader()).isEqualTo(cl); @@ -146,11 +144,10 @@ void testAdvancedClassloadingSampleString() throws Exception { @Test void testAdvancedClassloadingSampleURL() throws Exception { - URL[] urlClasspath = new URL[] { new File(testClassPath).toURL() }; - UIMAClassLoader cl = new UIMAClassLoader(urlClasspath, this.getClass().getClassLoader()); - Class testClass = null; + var urlClasspath = new URL[] { new File(testClassPath).toURL() }; + var cl = new UIMAClassLoader(urlClasspath, this.getClass().getClassLoader()); - testClass = cl.loadClass("org.apache.uima.internal.util.ClassloadingTestClass"); + var testClass = cl.loadClass("org.apache.uima.internal.util.ClassloadingTestClass"); assertThat(testClass).isNotNull(); assertThat(testClass.getClassLoader()).isEqualTo(cl); @@ -160,4 +157,29 @@ void testAdvancedClassloadingSampleURL() throws Exception { assertThat(testClass).isNotNull(); assertThat(testClass.getClassLoader()).isEqualTo(this.getClass().getClassLoader()); } + + @Test + void testLoadingSpiImplementationThroughUIMAClassLoaderIsBlocked() throws Exception { + var urlClasspath = new URL[] { new File(testClassPath).toURL() }; + var cl = new UIMAClassLoader(urlClasspath, this.getClass().getClassLoader()); + + var spiConfigurationFile = "META-INF/services/org.apache.uima.spi.TypeSystemProvider"; + var spiImplementationClass = "org.apache.uima.internal.util.UIMAClassLoaderTest_TypeSystemProvider"; + var filesToCheck = asList(spiConfigurationFile, + spiImplementationClass.replace('.', '/') + ".class"); + + try (var zipFile = new ZipFile(testClassPath)) { + for (var filePath : filesToCheck) { + var entry = zipFile.getEntry(filePath); + assertThat(entry).as("File %s should exist in the JAR", filePath).isNotNull(); + } + } + + var testClass = cl.loadClass(spiImplementationClass); + assertThat(testClass).isNotNull(); + assertThat(testClass.getClassLoader()).isEqualTo(this.getClass().getClassLoader()); + + var spiConfigurationFiles = list(cl.getResources(spiConfigurationFile)); + assertThat(spiConfigurationFiles).isEmpty(); + } } diff --git a/uimaj-core/src/test/java/org/apache/uima/internal/util/UIMAClassLoaderTest_TypeSystemProvider.java b/uimaj-core/src/test/java/org/apache/uima/internal/util/UIMAClassLoaderTest_TypeSystemProvider.java new file mode 100644 index 000000000..3d217d847 --- /dev/null +++ b/uimaj-core/src/test/java/org/apache/uima/internal/util/UIMAClassLoaderTest_TypeSystemProvider.java @@ -0,0 +1,36 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.uima.internal.util; + +import static java.util.Arrays.asList; + +import java.util.List; + +import org.apache.uima.jcas.cas.TOP; +import org.apache.uima.spi.SpiSentence; +import org.apache.uima.spi.SpiToken; +import org.apache.uima.spi.TypeSystemProvider; + +public class UIMAClassLoaderTest_TypeSystemProvider implements TypeSystemProvider { + + @Override + public List> listJCasClasses() { + return asList(SpiToken.class, SpiSentence.class); + } +} diff --git a/uimaj-core/src/test/resources/ClassLoaderTest/classLoadingTest.jar b/uimaj-core/src/test/resources/ClassLoaderTest/classLoadingTest.jar index 4ad650b21e580b299c4ddfe537e37f049df99ccc..3cc8bb908363fa8e235d3d63fa3ec07b2f016041 100644 GIT binary patch literal 1810 zcmWIWW@Zs#U|`^22dTEe(jeK5Ms7( zkZ;|=;-)5cyZ`I0s<-FYf9Gd-euMjhVvI;nwvcbP2d{5%Sn@=7Y#-2PQv`hNhR+S633(ME`Y}(H9{z-}7hvi3|+CCU` z2qdiLKUCJDSJe`@sHbMd^_4p5bF$3s{knR-&hftbA^-Ar?@&R#$@LQZlv?~(S3`hud7dIpKRVT*`mhv%cBj=3%d`OePOWU z6MV#>?0&Id#<1*G>f^Azzj+bC+Z;GKGE6)D$0lIVS}-#(crnNj2;NXnUq^6|`T&DC zwFnf#@gbE3slk=SC8@aqMfqi!Kw(IDhlX%6FsD6EP6y%A3T_5QmR~@Nz-eV|(Az9w zM-ki0Ih)Mfvajc_U8@@r#C?ugMa)ZO!J2IwHU|7rG{3DlcgIa$ZZh8k`5^X2m`T((n2NnnT3GUG{-Pn9bTWL95k&EUv6 z|M~XHY27?bz3a|xy)C?AYk40_Q(WPqSt}kUaVzR^tT|#8u7B#}gxw1gm$59`;h|}* zw0QO3{-g(fTSZ?S|DU{$FMf9B>v>;KRBcImwPLUE??BtYGsk(N{k~|ZhfC!x+|Tgh z*#4~_ZSLM}{dY_=GIH&~gNf<=`vtE5{Iyfw`Ho10wg2UmRqRKlr?&6V-SAP|Wl=<{ zO={x%M{yln=db(e_q})F%-lp-=^XZ(tBM=h--dtOvw3c@NQRngX3*8^Q@MBlU-o`+ z7whbV%avDuM(P!&?Y7kW>{lc<=QaQR#XrxM_>qT+JsvR@gPQ=D^(T#XZsW25$Q$-dA6FW)4a=RE{pjjYnfz>Qvme+%1c|A~; z*DEf_)Wb}5U>m;zZM%wTBf1H&LPHN!Xy_4_s<{HZ8JR?Yg&MA+4jLX%3LYeA1s}30 z*vdDADS<#H*`*!G8aZZ&V=?o&1l*Q}#tpzsiQgLH!UvY?Ss}R|BhEqY!Igqw?h#-( z;P?z!62SuxDLnzr#hR87=3=HMh`B%yGBh+Av0yV7J!RlZTR_`EX$!+wtZbm5WCcQD MCI*I=Ksy*10MR&x`v3p{ delta 136 zcmbQlH`om;LXS+!T>aN@+THoMvloAtcr|@6LW0D0=!w-KsGP|;aVVl0jQXP F0RYj-6+-|3 From 209a9a8d9cc744289ca4a3145d722fd8b07bc93f Mon Sep 17 00:00:00 2001 From: Richard Eckart de Castilho Date: Thu, 19 Dec 2024 13:18:08 +0100 Subject: [PATCH 6/6] Issue #431: Issue using SPI-enabled type systems embedded into PEARs - Added missing variable assignment --- .../org/apache/uima/resource/metadata/impl/Import_impl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/uimaj-core/src/main/java/org/apache/uima/resource/metadata/impl/Import_impl.java b/uimaj-core/src/main/java/org/apache/uima/resource/metadata/impl/Import_impl.java index a2b6fcfa5..4f6b8fc9a 100644 --- a/uimaj-core/src/main/java/org/apache/uima/resource/metadata/impl/Import_impl.java +++ b/uimaj-core/src/main/java/org/apache/uima/resource/metadata/impl/Import_impl.java @@ -134,7 +134,7 @@ private URL findResouceUrlByName(ResourceManager aResourceManager, String name) // If that fails, try loading through the SPIs if (url == null) { var cl = ClassLoaderUtils.findClassLoader(aResourceManager); - loadServicesSafely(TypeSystemProvider.class, cl) // + url = loadServicesSafely(TypeSystemProvider.class, cl) // .map(provider -> provider.findResourceUrl(name)) // .filter(Optional::isPresent) // .map(Optional::get) //