From 21414e6489cbe68c3b5a08f6e72593875b078cca Mon Sep 17 00:00:00 2001 From: Matt Gill Date: Mon, 13 Jan 2020 16:37:36 +0000 Subject: [PATCH 1/2] Add a WebappClassLoader synchronisation test Synchronisation issues in the WebappClassLoader have already been fixed, but not tested properly. This commit adds a test for the synchronisation issues that caused CUSTCOM-76. --- .../web/loader/WebappClassLoaderTest.java | 178 ++++++++++++++++++ 1 file changed, 178 insertions(+) create mode 100644 appserver/web/war-util/src/test/java/org/glassfish/web/loader/WebappClassLoaderTest.java diff --git a/appserver/web/war-util/src/test/java/org/glassfish/web/loader/WebappClassLoaderTest.java b/appserver/web/war-util/src/test/java/org/glassfish/web/loader/WebappClassLoaderTest.java new file mode 100644 index 00000000000..cb2bfd52ee9 --- /dev/null +++ b/appserver/web/war-util/src/test/java/org/glassfish/web/loader/WebappClassLoaderTest.java @@ -0,0 +1,178 @@ +/* + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER. + * + * Copyright (c) [2020] Payara Foundation and/or its affiliates. All rights reserved. + * + * The contents of this file are subject to the terms of either the GNU + * General Public License Version 2 only ("GPL") or the Common Development + * and Distribution License("CDDL") (collectively, the "License"). You + * may not use this file except in compliance with the License. You can + * obtain a copy of the License at + * https://github.com/payara/Payara/blob/master/LICENSE.txt + * See the License for the specific + * language governing permissions and limitations under the License. + * + * When distributing the software, include this License Header Notice in each + * file and include the License file at glassfish/legal/LICENSE.txt. + * + * GPL Classpath Exception: + * The Payara Foundation designates this particular file as subject to the "Classpath" + * exception as provided by the Payara Foundation in the GPL Version 2 section of the License + * file that accompanied this code. + * + * Modifications: + * If applicable, add the following below the License Header, with the fields + * enclosed by brackets [] replaced by your own identifying information: + * "Portions Copyright [year] [name of copyright owner]" + * + * Contributor(s): + * If you wish your version of this file to be governed by only the CDDL or + * only the GPL Version 2, indicate your decision by adding "[Contributor] + * elects to include this software in this distribution under the [CDDL or GPL + * Version 2] license." If you don't indicate a single choice of license, a + * recipient has the option to distribute your version of this file under + * either the CDDL, the GPL Version 2 or to extend the choice of license to + * its licensees as provided above. However, if you add GPL Version 2 code + * and therefore, elected the GPL Version 2 license, then the option applies + * only if the new code is made subject to such option by the copyright + * holder. + */ +package org.glassfish.web.loader; + +import java.io.File; +import java.io.IOException; +import java.net.URISyntaxException; +import java.util.Collections; +import java.util.LinkedList; +import java.util.List; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.jar.JarEntry; +import java.util.jar.JarFile; + +import org.apache.naming.resources.FileDirContext; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.Test; + +public class WebappClassLoaderTest { + + private static CyclicBarrier lock; + private static ExecutorService executor; + private static File junitJarFile; + + @BeforeClass + public static void setup() throws URISyntaxException { + // Run 3 methods at the same time, and make the pool large enough to increase + // the chance of a race condition + lock = new CyclicBarrier(3); + executor = Executors.newFixedThreadPool(60); + + // Fetch any JAR to use for classloading + junitJarFile = new File(Test.class.getProtectionDomain().getCodeSource().getLocation().toURI()); + } + + @AfterClass + public static void shutdown() { + if (executor != null) { + executor.shutdownNow(); + } + } + + @Test + public void check_findResourceInternalFromJars_thread_safety() throws Exception { + final ClassLoader classLoader = this.getClass().getClassLoader(); + final WebappClassLoader webappClassLoader = new WebappClassLoader(classLoader, null); + webappClassLoader.start(); + webappClassLoader.setResources(new FileDirContext()); + + CompletableFuture result = new CompletableFuture<>(); + + // Create the tasks, and have them each run at the same time + // using the cyclic barrier + Runnable lookupTask = waitAndDo(lock, result, () -> lookup(classLoader, webappClassLoader)); + Runnable addTask = waitAndDo(lock, result, () -> add(classLoader, webappClassLoader)); + Runnable closeTask = waitAndDo(lock, result, () -> webappClassLoader.closeJARs(true)); + + try { + // Run the methods at the same time + for (int i = 0; i < 100; i++) { + executor.execute(addTask); + executor.execute(lookupTask); + executor.execute(closeTask); + } + // Check to see if any completed exceptionally + Exception ex = result.get(200, TimeUnit.MILLISECONDS); + if (ex != null) { + throw ex; + } + } catch (TimeoutException ex) { + // Success! + } finally { + webappClassLoader.close(); + } + } + + private void add(ClassLoader realClassLoader, WebappClassLoader webappClassLoader) throws IOException { + List jarFiles = findJarFiles(realClassLoader); + + for (JarFile j : jarFiles) { + try { + webappClassLoader.addJar(junitJarFile.getName(), j, junitJarFile); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + } + + private void lookup(ClassLoader realClassLoader, WebappClassLoader webappClassLoader) throws Exception { + for (JarFile jarFile : findJarFiles(realClassLoader)) { + for (JarEntry entry : Collections.list(jarFile.entries())) { + webappClassLoader.findResource(entry.getName()); + // System.out.println("Looked up " + resourceEntry); + Thread.sleep(0, 100); + } + } + } + + private List findJarFiles(ClassLoader realClassLoader) throws IOException { + List jarFiles = new LinkedList<>(); + for (int i = 0; i < 10; i++) { + jarFiles.add(new JarFile(junitJarFile)); + } + return jarFiles; + } + + /** + * Generate a task that will wait on the passed cyclic barrier before running + * the passed task. Record the result in the passed future + * + * @param lock the lock to wait on before execution + * @param result where to store any encountered exceptions + * @param task the task to run + * @return a new task + */ + private static Runnable waitAndDo(final CyclicBarrier lock, final CompletableFuture result, + final ExceptionalRunnable task) { + return () -> { + try { + lock.await(); + task.run(); + } catch (Exception ex) { + result.complete(ex); + } + }; + } + + /** + * A runnable interface that allows exceptions + */ + @FunctionalInterface + private static interface ExceptionalRunnable { + void run() throws Exception; + } +} \ No newline at end of file From 7b6ac6abee24142153a37a2792c44a8c9178b0d3 Mon Sep 17 00:00:00 2001 From: Matt Gill Date: Tue, 14 Jan 2020 11:43:08 +0000 Subject: [PATCH 2/2] Improve the WebappClassLoaderTest The test could pass without any threads running in time when running on a slow machine. This commit makes sure that a minimum number of threads run before completing. A few other changes were made to make the test more reliable. --- .../web/loader/WebappClassLoaderTest.java | 83 ++++++++++--------- 1 file changed, 44 insertions(+), 39 deletions(-) diff --git a/appserver/web/war-util/src/test/java/org/glassfish/web/loader/WebappClassLoaderTest.java b/appserver/web/war-util/src/test/java/org/glassfish/web/loader/WebappClassLoaderTest.java index cb2bfd52ee9..6a0f341f3cc 100644 --- a/appserver/web/war-util/src/test/java/org/glassfish/web/loader/WebappClassLoaderTest.java +++ b/appserver/web/war-util/src/test/java/org/glassfish/web/loader/WebappClassLoaderTest.java @@ -39,6 +39,8 @@ */ package org.glassfish.web.loader; +import static org.junit.Assert.assertTrue; + import java.io.File; import java.io.IOException; import java.net.URISyntaxException; @@ -46,79 +48,83 @@ import java.util.LinkedList; import java.util.List; import java.util.concurrent.CompletableFuture; -import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; import java.util.jar.JarEntry; import java.util.jar.JarFile; import org.apache.naming.resources.FileDirContext; -import org.junit.AfterClass; -import org.junit.BeforeClass; +import org.junit.After; +import org.junit.Before; import org.junit.Test; public class WebappClassLoaderTest { - private static CyclicBarrier lock; - private static ExecutorService executor; - private static File junitJarFile; + private static final int EXECUTION_COUNT = 100; + + private CountDownLatch latch; - @BeforeClass - public static void setup() throws URISyntaxException { + private ExecutorService executor; + private File junitJarFile; + + @Before + public void setup() throws URISyntaxException { // Run 3 methods at the same time, and make the pool large enough to increase // the chance of a race condition - lock = new CyclicBarrier(3); executor = Executors.newFixedThreadPool(60); + // Require a minimum number of executions before completing + latch = new CountDownLatch(EXECUTION_COUNT); + // Fetch any JAR to use for classloading junitJarFile = new File(Test.class.getProtectionDomain().getCodeSource().getLocation().toURI()); } - @AfterClass - public static void shutdown() { + @After + public void shutdown() throws InterruptedException { if (executor != null) { executor.shutdownNow(); + assertTrue("Executor could not shutdown. This could mean there is a deadlock.", + executor.awaitTermination(10, TimeUnit.SECONDS)); } } @Test public void check_findResourceInternalFromJars_thread_safety() throws Exception { - final ClassLoader classLoader = this.getClass().getClassLoader(); - final WebappClassLoader webappClassLoader = new WebappClassLoader(classLoader, null); + final WebappClassLoader webappClassLoader = new WebappClassLoader(getClass().getClassLoader(), null); webappClassLoader.start(); webappClassLoader.setResources(new FileDirContext()); - CompletableFuture result = new CompletableFuture<>(); + CompletableFuture result = new CompletableFuture<>(); - // Create the tasks, and have them each run at the same time - // using the cyclic barrier - Runnable lookupTask = waitAndDo(lock, result, () -> lookup(classLoader, webappClassLoader)); - Runnable addTask = waitAndDo(lock, result, () -> add(classLoader, webappClassLoader)); - Runnable closeTask = waitAndDo(lock, result, () -> webappClassLoader.closeJARs(true)); + // Create the tasks to run + Runnable lookupTask = waitAndDo(result, () -> lookup(webappClassLoader)); + Runnable addTask = waitAndDo(result, () -> add(webappClassLoader)); + Runnable closeTask = waitAndDo(result, () -> webappClassLoader.closeJARs(true)); try { // Run the methods at the same time - for (int i = 0; i < 100; i++) { + for (int i = 0; i < EXECUTION_COUNT; i++) { executor.execute(addTask); executor.execute(lookupTask); executor.execute(closeTask); } - // Check to see if any completed exceptionally - Exception ex = result.get(200, TimeUnit.MILLISECONDS); - if (ex != null) { - throw ex; - } - } catch (TimeoutException ex) { - // Success! + + // Wait for tasks to execute + assertTrue("The tasks didn't finish in the allowed time.", + latch.await(20, TimeUnit.SECONDS)); + + // Check to see if any tasks completed exceptionally + result.getNow(null); } finally { webappClassLoader.close(); } } - private void add(ClassLoader realClassLoader, WebappClassLoader webappClassLoader) throws IOException { - List jarFiles = findJarFiles(realClassLoader); + private void add(WebappClassLoader webappClassLoader) throws IOException { + List jarFiles = findJarFiles(); for (JarFile j : jarFiles) { try { @@ -129,8 +135,8 @@ private void add(ClassLoader realClassLoader, WebappClassLoader webappClassLoade } } - private void lookup(ClassLoader realClassLoader, WebappClassLoader webappClassLoader) throws Exception { - for (JarFile jarFile : findJarFiles(realClassLoader)) { + private void lookup(WebappClassLoader webappClassLoader) throws Exception { + for (JarFile jarFile : findJarFiles()) { for (JarEntry entry : Collections.list(jarFile.entries())) { webappClassLoader.findResource(entry.getName()); // System.out.println("Looked up " + resourceEntry); @@ -139,7 +145,7 @@ private void lookup(ClassLoader realClassLoader, WebappClassLoader webappClassLo } } - private List findJarFiles(ClassLoader realClassLoader) throws IOException { + private List findJarFiles() throws IOException { List jarFiles = new LinkedList<>(); for (int i = 0; i < 10; i++) { jarFiles.add(new JarFile(junitJarFile)); @@ -151,19 +157,18 @@ private List findJarFiles(ClassLoader realClassLoader) throws IOExcepti * Generate a task that will wait on the passed cyclic barrier before running * the passed task. Record the result in the passed future * - * @param lock the lock to wait on before execution * @param result where to store any encountered exceptions * @param task the task to run * @return a new task */ - private static Runnable waitAndDo(final CyclicBarrier lock, final CompletableFuture result, - final ExceptionalRunnable task) { + private Runnable waitAndDo(final CompletableFuture result, final ExceptionalRunnable task) { return () -> { try { - lock.await(); task.run(); } catch (Exception ex) { - result.complete(ex); + result.completeExceptionally(ex); + } finally { + latch.countDown(); } }; } @@ -172,7 +177,7 @@ private static Runnable waitAndDo(final CyclicBarrier lock, final CompletableFut * A runnable interface that allows exceptions */ @FunctionalInterface - private static interface ExceptionalRunnable { + private interface ExceptionalRunnable { void run() throws Exception; } } \ No newline at end of file