[LOGGING-186] Replace custom code with ServiceLoader call (#171)

* Replace custom code with `ServiceLoader` call

* Files should end with an empty line

* Files should end with an empty line

* Comment empty block

* Comment empty block

---------

Co-authored-by: Gary Gregory <garydgregory@users.noreply.github.com>

pom.xml

@@ -391,6 +391,9 @@ under the License.
             </goals>
             <configuration>
               <runOrder>${failsafe.runorder}</runOrder>
+              <excludes>
+                <exclude>org/apache/commons/logging/serviceloader/**</exclude>
+              </excludes>
               <includes>
                 <include>**/*TestCase.java</include>
               </includes>
@@ -408,6 +411,24 @@ under the License.
               </systemPropertyVariables>
             </configuration>
           </execution>
+          <!--
+            - The ServiceLoaderTestCase uses an alternative implementation of LogFactory,
+            - therefore it requires a separate execution.
+            -->
+          <execution>
+            <id>serviceLoader-test</id>
+            <goals>
+              <goal>integration-test</goal>
+            </goals>
+            <configuration>
+              <additionalClasspathElements>
+                <additionalClasspathElement>${project.build.testOutputDirectory}/serviceloader</additionalClasspathElement>
+              </additionalClasspathElements>
+              <includes>
+                <include>org/apache/commons/logging/serviceloader/*TestCase.java</include>
+              </includes>
+            </configuration>
+          </execution>
         </executions>
       </plugin>
 

src/changes/changes.xml

@@ -65,7 +65,10 @@ The <action> type attribute can be add,update,fix,remove.
       </action>
       <action type="fix" dev="ggregory" due-to="step-security-bot, Gary Gregory">
         [StepSecurity] ci: Harden GitHub Actions #145.
-      </action>            
+      </action>
+      <action issue="LOGGING-185" type="fix" dev="pkarwasz" due-to="Piotr P. Karwasz">
+        Replace custom code with `ServiceLoader` call.
+      </action>
       <!-- UPDATES -->
       <action dev="ggregory" type="update" due-to="Gary Gregory">
         Bump Java from 6 to 8.

src/main/java/org/apache/commons/logging/LogFactory.java

@@ -30,7 +30,10 @@ import java.security.AccessController;
 import java.security.PrivilegedAction;
 import java.util.Enumeration;
 import java.util.Hashtable;
+import java.util.Iterator;
 import java.util.Properties;
+import java.util.ServiceConfigurationError;
+import java.util.ServiceLoader;
 
 /**
  * Factory for creating {@link Log} instances, with discovery and
@@ -189,6 +192,12 @@ public abstract class LogFactory {
      */
     private static final WeakReference<ClassLoader> thisClassLoaderRef;
 
+    /**
+     * Maximum number of {@link ServiceLoader} errors to ignore, while
+     * looking for an implementation.
+     */
+    private static final int MAX_BROKEN_SERVICES = 3;
+
     // ----------------------------------------------------------- Constructors
 
     /**
@@ -507,42 +516,25 @@ public abstract class LogFactory {
 
         if (factory == null) {
             if (isDiagnosticsEnabled()) {
-                logDiagnostic("[LOOKUP] Looking for a resource file of name [" + SERVICE_ID +
+                logDiagnostic("[LOOKUP] Using ServiceLoader  to define the LogFactory subclass to use...");
-                              "] to define the LogFactory subclass to use...");
             }
             try {
-                final InputStream is = getResourceAsStream(contextClassLoader, SERVICE_ID);
+                final ServiceLoader<LogFactory> serviceLoader = ServiceLoader.load(LogFactory.class);
+                final Iterator<LogFactory> iterator = serviceLoader.iterator();
 
-                if ( is != null ) {
+                int i = MAX_BROKEN_SERVICES;
-                    // This code is needed by EBCDIC and other strange systems.
+                while (factory == null && i-- > 0) {
-                    // It's a fix for bugs reported in xerces
-                    BufferedReader rd;
                     try {
-                        rd = new BufferedReader(new InputStreamReader(is, "UTF-8"));
+                        if (iterator.hasNext()) {
-                    } catch (final java.io.UnsupportedEncodingException e) {
+                            factory = iterator.next();
-                        rd = new BufferedReader(new InputStreamReader(is));
+                        }
-                    }
+                    } catch (final ServiceConfigurationError | LinkageError ex) {
-
+                        if (isDiagnosticsEnabled()) {
-                    String factoryClassName;
+                            logDiagnostic("[LOOKUP] An exception occurred while trying to find an" +
-                    try {
+                                    " instance of LogFactory" +
-                        factoryClassName = rd.readLine();
+                                    ": [" + trim(ex.getMessage()) +
-                    } finally {
+                                    "]. Trying alternative implementations...");
-                        rd.close();
-                    }
-
-                    if (factoryClassName != null && ! factoryClassName.isEmpty()) {
-                        if (isDiagnosticsEnabled()) {
-                            logDiagnostic("[LOOKUP]  Creating an instance of LogFactory class " +
-                                          factoryClassName +
-                                          " as specified by file '" + SERVICE_ID +
-                                          "' which was present in the path of the context classloader.");
                         }
-                        factory = newFactory(factoryClassName, baseClassLoader, contextClassLoader );
-                    }
-                } else {
-                    // is == null
-                    if (isDiagnosticsEnabled()) {
-                        logDiagnostic("[LOOKUP] No resource file with name '" + SERVICE_ID + "' found.");
                     }
                 }
             } catch (final Exception ex) {

src/test/java/org/apache/commons/logging/serviceloader/ServiceLoaderTestCase.java

@@ -0,0 +1,30 @@
+/*
+ * 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.commons.logging.serviceloader;
+
+import junit.framework.TestCase;
+import org.apache.commons.logging.LogFactory;
+import org.apache.commons.logging.serviceloader.internal.DummyLogFactory;
+
+public class ServiceLoaderTestCase extends TestCase {
+
+    public void testServiceLoader() {
+        final LogFactory factory = LogFactory.getFactory();
+        assertTrue("Wrong factory retrieved through ServiceLoader: " + factory.getClass().getName(),
+                factory instanceof DummyLogFactory);
+    }
+}

src/test/java/org/apache/commons/logging/serviceloader/internal/DummyLogFactory.java

@@ -0,0 +1,58 @@
+/*
+ * 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.commons.logging.serviceloader.internal;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogConfigurationException;
+import org.apache.commons.logging.LogFactory;
+
+public class DummyLogFactory extends LogFactory {
+    @Override
+    public Object getAttribute(String name) {
+        return null;
+    }
+
+    @Override
+    public String[] getAttributeNames() {
+        return new String[0];
+    }
+
+    @Override
+    public Log getInstance(Class clazz) throws LogConfigurationException {
+        return null;
+    }
+
+    @Override
+    public Log getInstance(String name) throws LogConfigurationException {
+        return null;
+    }
+
+    @Override
+    public void release() {
+        // empty
+    }
+
+    @Override
+    public void removeAttribute(String name) {
+        // empty
+    }
+
+    @Override
+    public void setAttribute(String name, Object value) {
+        // empty
+    }
+}

src/test/java/org/apache/commons/logging/serviceloader/internal/ThrowingLogFactory.java

@@ -0,0 +1,25 @@
+/*
+ * 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.commons.logging.serviceloader.internal;
+
+/**
+ * A common ServiceLoader error is finding a class that implements LogFactory from a different classloader.
+ * This class should emulate that behavior.
+ */
+public class ThrowingLogFactory {
+  // empty
+}

src/test/resources/serviceloader/META-INF/services/org.apache.commons.logging.LogFactory

@@ -0,0 +1,25 @@
+# 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.
+##
+# Version 1.2 of Apache Commons Logging did not support comments in the service file.
+#
+# A class that does not implement LogFactory to emulate services the implement
+# LogFactory for another classloader
+org.apache.commons.logging.serviceloader.internal.ThrowingLogFactory
+##
+# A proper LogFactory
+org.apache.commons.logging.serviceloader.internal.DummyLogFactory

src/test/resources/serviceloader/README.txt

@@ -0,0 +1 @@
+Contains resources used exclusively by `ServiceLoaderTestCase`