This is an automated email from the ASF dual-hosted git repository.
markt-asf pushed a commit to branch 10.1.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/10.1.x by this push:
new 0a7a97ec60 Review "Ignore unusable JULI Log service providers instead
of failing"
0a7a97ec60 is described below
commit 0a7a97ec6072198dae1eb5e78b64a2d86faf45e7
Author: Mark Thomas <[email protected]>
AuthorDate: Fri Nov 6 15:42:33 2026 +0000
Review "Ignore unusable JULI Log service providers instead of failing"
Handle some additional cases of unusable service providers
Co-authored by: Codex / GPT-6.1-Sol
---
java/org/apache/juli/logging/LogFactory.java | 63 ++++++-
test/org/apache/juli/logging/TestLogFactory.java | 194 +++++++++++++++++++--
.../logging/provider/TesterLogFactoryProvider.java | 44 +++++
3 files changed, 278 insertions(+), 23 deletions(-)
diff --git a/java/org/apache/juli/logging/LogFactory.java
b/java/org/apache/juli/logging/LogFactory.java
index 285b5672f0..a299195374 100644
--- a/java/org/apache/juli/logging/LogFactory.java
+++ b/java/org/apache/juli/logging/LogFactory.java
@@ -16,8 +16,13 @@
*/
package org.apache.juli.logging;
+import java.io.IOException;
import java.lang.reflect.Constructor;
+import java.lang.reflect.Modifier;
+import java.net.URL;
import java.nio.file.FileSystems;
+import java.util.Collections;
+import java.util.Enumeration;
import java.util.Iterator;
import java.util.ServiceConfigurationError;
import java.util.ServiceLoader;
@@ -75,26 +80,70 @@ public class LogFactory {
// constructor taking the String name. Providers that cannot be
// loaded or that do not expose the required constructor are
// ignored, so a malformed provider does not prevent the discovery
- // of a usable one, nor the fall back to DirectJDKLog. Note that,
- // as the ServiceLoader requires, a provider also needs a public
- // no-argument constructor to be considered at all.
- ServiceLoader<Log> logLoader = ServiceLoader.load(Log.class);
+ // of a usable one, nor the fall back to DirectJDKLog. Class-path
+ // providers need a public no-argument constructor; named-module
+ // providers may instead use a factory method.
+ ServiceLoader<Log> logLoader = ServiceLoader.load(Log.class,
+ new
ServiceLoaderClassLoader(Thread.currentThread().getContextClassLoader()));
Constructor<? extends Log> m = null;
Iterator<ServiceLoader.Provider<Log>> providers =
logLoader.stream().iterator();
while (m == null) {
+ ServiceLoader.Provider<Log> provider;
try {
if (!providers.hasNext()) {
break;
}
- m = providers.next().type().getConstructor(String.class);
- } catch (ReflectiveOperationException | SecurityException |
ServiceConfigurationError e) {
- // Not a usable provider. Ignore it and look at the next one.
+ provider = providers.next();
+ } catch (ServiceConfigurationError | SecurityException |
LinkageError e) {
+ // An individual service file or provider may be unusable. The
resource enumeration has already
+ // advanced past it, so continue with the next file or
provider.
+ continue;
+ }
+ try {
+ Class<? extends Log> type = provider.type();
+ if (type.getModule().isNamed()) {
+ // For a factory-method provider, type() is the method's
declared return type, which may be
+ // the Log interface or a base class rather than the
implementation whose constructor we need.
+ type = provider.get().getClass();
+ }
+ Constructor<? extends Log> candidate =
type.getConstructor(String.class);
+ // Instances are created later, by reflection from this class.
A provider that is not accessible
+ // to it would fail every call of getInstance(), so treat it
as unusable now.
+ if (Modifier.isPublic(type.getModifiers()) &&
+ type.getModule().isExported(type.getPackageName(),
LogFactory.class.getModule())) {
+ m = candidate;
+ }
+ } catch (ReflectiveOperationException | SecurityException |
ServiceConfigurationError | LinkageError e) {
+ // Not a usable provider, for example because a type it refers
to is not available. Ignore it and
+ // look at the next one.
}
}
discoveredLogConstructor = m;
}
+ /**
+ * Delegate service discovery to the context class loader, but turn a
failed resource lookup into an empty
+ * enumeration. Unlike a failure reading an individual service file, a
failure obtaining the enumeration leaves
+ * ServiceLoader unable to advance, so retrying its iterator would repeat
the same lookup indefinitely.
+ */
+ private static class ServiceLoaderClassLoader extends ClassLoader {
+
+ private ServiceLoaderClassLoader(ClassLoader parent) {
+ super(parent == null ? ClassLoader.getSystemClassLoader() :
parent);
+ }
+
+ @Override
+ public Enumeration<URL> getResources(String name) {
+ try {
+ return super.getResources(name);
+ } catch (IOException | SecurityException | LinkageError e) {
+ return Collections.emptyEnumeration();
+ }
+ }
+ }
+
+
// --------------------------------------------------------- Public Methods
// only those 2 methods need to change to use a different direct logger.
diff --git a/test/org/apache/juli/logging/TestLogFactory.java
b/test/org/apache/juli/logging/TestLogFactory.java
index 69d595ff79..9920f45b4e 100644
--- a/test/org/apache/juli/logging/TestLogFactory.java
+++ b/test/org/apache/juli/logging/TestLogFactory.java
@@ -17,15 +17,28 @@
package org.apache.juli.logging;
import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.lang.module.Configuration;
+import java.lang.module.ModuleFinder;
import java.net.URL;
import java.net.URLClassLoader;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
+import java.util.Collections;
+import java.util.Enumeration;
+import java.util.List;
+import java.util.Set;
+
+import javax.tools.JavaCompiler;
+import javax.tools.ToolProvider;
import org.junit.Assert;
import org.junit.Test;
+import org.apache.juli.logging.provider.TesterLogFactoryProvider;
+
public class TestLogFactory {
private static final String NO_STRING_CONSTRUCTOR =
TesterLogNoStringConstructor.class.getName();
@@ -101,6 +114,150 @@ public class TestLogFactory {
}
+ @Test
+ public void testResourceLookupFailureStopsDiscovery() throws Exception {
+ doTestResourceLookupFailureStopsDiscovery(false);
+ }
+
+
+ @Test
+ public void testDeniedResourceLookupStopsDiscovery() throws Exception {
+ doTestResourceLookupFailureStopsDiscovery(true);
+ }
+
+
+ private void doTestResourceLookupFailureStopsDiscovery(boolean denied)
throws Exception {
+ URL[] urls = { codeSourceOf(LogFactory.class),
codeSourceOf(TestLogFactory.class) };
+ int[] lookups = new int[1];
+ try (URLClassLoader loader = new URLClassLoader(urls,
ClassLoader.getPlatformClassLoader()) {
+ @Override
+ public Enumeration<URL> getResources(String name) throws
IOException {
+ if (name.equals("META-INF/services/" + Log.class.getName())) {
+ // Fail a repeated lookup immediately rather than letting
a broken implementation loop forever.
+ Assert.assertEquals("The failing resource enumeration was
retried", 1, ++lookups[0]);
+ if (denied) {
+ throw new SecurityException("Test resource lookup
denial");
+ }
+ throw new IOException("Test resource lookup failure");
+ }
+ return super.getResources(name);
+ }
+ }) {
+ Assert.assertEquals(DIRECT_JDK_LOG,
getLogFromFactory(loader).getClass().getName());
+ Assert.assertEquals(1, lookups[0]);
+ }
+ }
+
+
+ @Test
+ public void testUnreadableServiceFilesDoNotHideUsableProvider() throws
Exception {
+ Path tempDir =
Files.createTempDirectory("juli-logfactory-unreadable-test");
+ try {
+ Path service = tempDir.resolve("readable.services");
+ Files.writeString(service, STRING_CONSTRUCTOR +
System.lineSeparator(), StandardCharsets.UTF_8);
+ List<URL> resources =
List.of(tempDir.resolve("missing.services").toUri().toURL(),
+ tempDir.resolve("also-missing.services").toUri().toURL(),
service.toUri().toURL());
+ URL[] urls = { codeSourceOf(LogFactory.class),
codeSourceOf(TestLogFactory.class) };
+ try (URLClassLoader loader = new URLClassLoader(urls,
ClassLoader.getPlatformClassLoader()) {
+ @Override
+ public Enumeration<URL> getResources(String name) throws
IOException {
+ if (name.equals("META-INF/services/" +
Log.class.getName())) {
+ return Collections.enumeration(resources);
+ }
+ return super.getResources(name);
+ }
+ }) {
+ Assert.assertEquals(STRING_CONSTRUCTOR,
getLogFromFactory(loader).getClass().getName());
+ }
+ } finally {
+ deleteRecursively(tempDir.toFile());
+ }
+ }
+
+
+ @Test
+ public void testModuleFactoryProviderIsDiscovered() throws Exception {
+ doTestModuleFactoryProvider(TesterLogFactoryProvider.class, true);
+ }
+
+
+ @Test
+ public void testModuleFactoryProviderReturningBaseClassIsDiscovered()
throws Exception {
+
doTestModuleFactoryProvider(TesterLogFactoryProvider.BaseFactory.class, true);
+ }
+
+
+ /*
+ * A provider whose package is not exported to the logging module cannot
be instantiated by it, so it must be
+ * ignored at discovery rather than make every later call to getLog() fail.
+ */
+ @Test
+ public void testModuleProviderInUnexportedPackageIsIgnored() throws
Exception {
+ doTestModuleFactoryProvider(TesterLogFactoryProvider.class, false);
+ }
+
+
+ private void doTestModuleFactoryProvider(Class<?> providerClass, boolean
exported) throws Exception {
+ Path tempDir =
Files.createTempDirectory("juli-logfactory-module-test");
+ try {
+ Path modules = tempDir.resolve("modules");
+ Path juli = modules.resolve("test.juli");
+ copyClasses(juli, Log.class, LogFactory.class,
LogConfigurationException.class, DirectJDKLog.class,
+ TesterLogStringConstructor.class);
+ copyClasses(juli, LogFactory.class.getDeclaredClasses());
+ compileModule(tempDir.resolve("juli-src"), juli, "test.juli",
+ "module test.juli { requires java.logging; exports
org.apache.juli.logging; " +
+ "uses org.apache.juli.logging.Log; }", modules);
+
+ Path provider = modules.resolve("test.provider");
+ copyClasses(provider, TesterLogFactoryProvider.class,
TesterLogFactoryProvider.NamedLog.class,
+ TesterLogFactoryProvider.BaseFactory.class);
+ compileModule(tempDir.resolve("provider-src"), provider,
"test.provider",
+ "module test.provider { requires test.juli; " +
+ (exported ? "exports
org.apache.juli.logging.provider; " : "") +
+ "provides org.apache.juli.logging.Log with " +
providerClass.getCanonicalName() +
+ "; }", modules);
+
+ ModuleFinder finder = ModuleFinder.of(modules);
+ Configuration configuration =
ModuleLayer.boot().configuration().resolve(finder, ModuleFinder.of(),
+ Set.of("test.provider"));
+ ModuleLayer layer =
ModuleLayer.boot().defineModulesWithOneLoader(configuration,
+ ClassLoader.getPlatformClassLoader());
+ Object log = getLogFromFactory(layer.findLoader("test.provider"));
+ Assert.assertEquals(exported ?
TesterLogFactoryProvider.NamedLog.class.getName() : DIRECT_JDK_LOG,
+ log.getClass().getName());
+ } finally {
+ deleteRecursively(tempDir.toFile());
+ }
+ }
+
+
+ private static void copyClasses(Path directory, Class<?>... classes)
throws IOException {
+ for (Class<?> clazz : classes) {
+ String resource = clazz.getName().replace('.', '/') + ".class";
+ Path target = directory.resolve(resource);
+ Files.createDirectories(target.getParent());
+ try (InputStream input = clazz.getResourceAsStream('/' +
resource)) {
+ Assert.assertNotNull(resource, input);
+ Files.copy(input, target);
+ }
+ }
+ }
+
+
+ private static void compileModule(Path sources, Path output, String name,
String descriptor, Path modules)
+ throws IOException {
+ Files.createDirectories(sources);
+ Path source = sources.resolve("module-info.java");
+ Files.writeString(source, descriptor, StandardCharsets.UTF_8);
+ JavaCompiler compiler = ToolProvider.getSystemJavaCompiler();
+ Assert.assertNotNull("A JDK compiler is required", compiler);
+ Assert.assertEquals("Module compilation failed", 0, compiler.run(null,
null, null,
+ "--module-path", modules.toString(), "--patch-module", name +
'=' + output,
+ "-d", output.toString(), source.toString()));
+ }
+
+
/*
* Loads a fresh copy of the LogFactory in an isolated class loader in
which the given
* names are registered as the only providers of the Log service, calls
getLog() twice
@@ -125,22 +282,7 @@ public class TestLogFactory {
URL[] urls = { codeSourceOf(LogFactory.class),
codeSourceOf(TestLogFactory.class),
tempDir.toUri().toURL() };
try (URLClassLoader isolatedLoader = new URLClassLoader(urls,
ClassLoader.getPlatformClassLoader())) {
- Thread currentThread = Thread.currentThread();
- ClassLoader originalLoader =
currentThread.getContextClassLoader();
- // The ServiceLoader used by LogFactory looks at the context
class loader
- currentThread.setContextClassLoader(isolatedLoader);
- try {
- Class<?> isolatedFactory =
isolatedLoader.loadClass(LogFactory.class.getName());
- Object result = null;
- // Call twice, the second call must not fail with
NoClassDefFoundError
- for (int i = 0; i < 2; i++) {
- result = isolatedFactory.getMethod("getLog",
String.class).invoke(null, "tester");
- Assert.assertNotNull(result);
- }
- return result;
- } finally {
- currentThread.setContextClassLoader(originalLoader);
- }
+ return getLogFromFactory(isolatedLoader);
}
} finally {
deleteRecursively(tempDir.toFile());
@@ -148,6 +290,26 @@ public class TestLogFactory {
}
+ private static Object getLogFromFactory(ClassLoader loader) throws
Exception {
+ Thread currentThread = Thread.currentThread();
+ ClassLoader originalLoader = currentThread.getContextClassLoader();
+ // The ServiceLoader used by LogFactory looks at the context class
loader.
+ currentThread.setContextClassLoader(loader);
+ try {
+ Class<?> isolatedFactory =
loader.loadClass(LogFactory.class.getName());
+ Object result = null;
+ // Call twice, the second call must not fail with
NoClassDefFoundError.
+ for (int i = 0; i < 2; i++) {
+ result = isolatedFactory.getMethod("getLog",
String.class).invoke(null, "tester");
+ Assert.assertNotNull(result);
+ }
+ return result;
+ } finally {
+ currentThread.setContextClassLoader(originalLoader);
+ }
+ }
+
+
private static URL codeSourceOf(Class<?> clazz) throws Exception {
return clazz.getProtectionDomain().getCodeSource().getLocation();
}
diff --git
a/test/org/apache/juli/logging/provider/TesterLogFactoryProvider.java
b/test/org/apache/juli/logging/provider/TesterLogFactoryProvider.java
new file mode 100644
index 0000000000..8574e246e1
--- /dev/null
+++ b/test/org/apache/juli/logging/provider/TesterLogFactoryProvider.java
@@ -0,0 +1,44 @@
+/*
+ * 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.juli.logging.provider;
+
+import org.apache.juli.logging.Log;
+import org.apache.juli.logging.TesterLogStringConstructor;
+
+/**
+ * Named-module service provider whose factory declares the service interface
as its return type.
+ */
+public class TesterLogFactoryProvider {
+
+ public static Log provider() {
+ return new NamedLog("initial");
+ }
+
+ public static class BaseFactory {
+
+ public static TesterLogStringConstructor provider() {
+ return new NamedLog("initial");
+ }
+ }
+
+ public static class NamedLog extends TesterLogStringConstructor {
+
+ public NamedLog(String name) {
+ super(name);
+ }
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
