From aef76767d5865333b619ed3f9f543269db43f95a Mon Sep 17 00:00:00 2001 From: Robert Stupp Date: Fri, 30 Aug 2024 12:25:26 +0200 Subject: [PATCH] "Thread per test class" via `MultiEnvTestEngine` See #9441 for a complete description of the underlying problem. TL;DR is: `ThreadLocal`s from various 3rd party libraries leak into the single `Test worker` thread that runs all the tests, resulting in TL objects/suppliers from the various Quarkus test class loaders, eventually leading to nasty OOMs. This change updates the `MultiEnvTestEngine` by using the new `ThreadPerTestClassExecutionExecutorService` and also "assimilate" really all tests, even the non-multi-env tests, so that those also run on a thread per test-class. The logic to distinguish multi-env from non-multi-env tests via `MultiEnvExtensionRegistry.registerExtension()` via test discovery is not 100% deterministic, it can add multi-env tests to the non-multi-env tests, so an additional check is needed there. Since each test class runs on "its own thread", the `ThreadLocal`s are registered on that thread. Once the test class finishes, the thread is disposed and its thread locals become eligible for garbage collection, which is what is needed. The bump of the max-heap size for test workers is also reduced back to 2g (was changed in #9433). Fixes #9441 --- .../src/main/kotlin/nessie-testing.gradle.kts | 2 +- .../engine/MultiEnvExtensionRegistry.java | 35 +++++- .../junit/engine/MultiEnvTestEngine.java | 69 ++++++++--- .../junit/engine/MultiEnvTestFilter.java | 40 ++---- ...dPerTestClassExecutionExecutorService.java | 117 ++++++++++++++++++ ...ThreadPerTestClassExecutionTestEngine.java | 85 +++++++++++++ 6 files changed, 300 insertions(+), 48 deletions(-) create mode 100644 testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/ThreadPerTestClassExecutionExecutorService.java create mode 100644 testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/ThreadPerTestClassExecutionTestEngine.java diff --git a/build-logic/src/main/kotlin/nessie-testing.gradle.kts b/build-logic/src/main/kotlin/nessie-testing.gradle.kts index 89167e1108d..4f435de4b3f 100644 --- a/build-logic/src/main/kotlin/nessie-testing.gradle.kts +++ b/build-logic/src/main/kotlin/nessie-testing.gradle.kts @@ -108,7 +108,7 @@ tasks.withType().configureEach { ) minHeapSize = if (testHeapSize != null) testHeapSize as String else "768m" - maxHeapSize = if (testHeapSize != null) testHeapSize as String else "3g" + maxHeapSize = if (testHeapSize != null) testHeapSize as String else "2g" } else if (testHeapSize != null) { minHeapSize = testHeapSize!! maxHeapSize = testHeapSize!! diff --git a/testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvExtensionRegistry.java b/testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvExtensionRegistry.java index b886d03f2f5..617db374e89 100644 --- a/testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvExtensionRegistry.java +++ b/testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvExtensionRegistry.java @@ -17,10 +17,14 @@ import java.util.Arrays; import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.stream.Stream; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.extension.Extension; import org.junit.jupiter.engine.config.DefaultJupiterConfiguration; +import org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor; import org.junit.jupiter.engine.extension.MutableExtensionRegistry; import org.junit.platform.commons.util.AnnotationUtils; @@ -33,17 +37,40 @@ public class MultiEnvExtensionRegistry { private final MutableExtensionRegistry registry; + private final Set probablyNotMultiEnv = new LinkedHashSet<>(); + public MultiEnvExtensionRegistry() { this.registry = MutableExtensionRegistry.createRegistryWithDefaultExtensions( new DefaultJupiterConfiguration(new EmptyConfigurationParameters())); } - public void registerExtensions(Class testClass) { - AnnotationUtils.findRepeatableAnnotations(testClass, ExtendWith.class).stream() - .flatMap(e -> Arrays.stream(e.value())) - .filter(MultiEnvTestExtension.class::isAssignableFrom) + public void registerExtensions(ClassBasedTestDescriptor descriptor) { + AtomicBoolean multiEnv = new AtomicBoolean(false); + + findMultiEnvExtensions(descriptor) + .peek(x -> multiEnv.set(true)) .forEach(registry::registerExtension); + + if (!multiEnv.get()) { + probablyNotMultiEnv.add(descriptor); + } + } + + public boolean isMultiEnvClass(ClassBasedTestDescriptor descriptor) { + return findMultiEnvExtensions(descriptor).findFirst().isPresent(); + } + + private Stream> findMultiEnvExtensions( + ClassBasedTestDescriptor descriptor) { + Class testClass = descriptor.getTestClass(); + return AnnotationUtils.findRepeatableAnnotations(testClass, ExtendWith.class).stream() + .flatMap(e -> Arrays.stream(e.value())) + .filter(MultiEnvTestExtension.class::isAssignableFrom); + } + + public Stream probablyNotMultiEnv() { + return probablyNotMultiEnv.stream(); } public Stream stream() { diff --git a/testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestEngine.java b/testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestEngine.java index db360fc8c6e..ceb1c40b9b6 100644 --- a/testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestEngine.java +++ b/testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestEngine.java @@ -26,6 +26,7 @@ import org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor; import org.junit.jupiter.engine.descriptor.JupiterEngineDescriptor; import org.junit.jupiter.engine.discovery.DiscoverySelectorResolver; +import org.junit.platform.engine.ConfigurationParameters; import org.junit.platform.engine.EngineDiscoveryRequest; import org.junit.platform.engine.ExecutionRequest; import org.junit.platform.engine.TestDescriptor; @@ -51,7 +52,8 @@ public class MultiEnvTestEngine implements TestEngine { private static final MultiEnvExtensionRegistry registry = new MultiEnvExtensionRegistry(); - private final JupiterTestEngine delegate = new JupiterTestEngine(); + private final ThreadPerTestClassExecutionTestEngine delegate = + new ThreadPerTestClassExecutionTestEngine(); static MultiEnvExtensionRegistry registry() { return registry; @@ -76,36 +78,36 @@ public TestDescriptor discover(EngineDiscoveryRequest discoveryRequest, UniqueId preliminaryResult.accept( descriptor -> { if (descriptor instanceof ClassBasedTestDescriptor) { - Class testClass = ((ClassBasedTestDescriptor) descriptor).getTestClass(); - registry.registerExtensions(testClass); + registry.registerExtensions(((ClassBasedTestDescriptor) descriptor)); } }); + ConfigurationParameters configurationParameters = + discoveryRequest.getConfigurationParameters(); + // JupiterEngineDescriptor must be the root, that's what the JUnit Jupiter engine // implementation expects. - JupiterEngineDescriptor multiEnvDescriptor = + JupiterEngineDescriptor multiEnvRootDescriptor = new JupiterEngineDescriptor( - uniqueId, - new DefaultJupiterConfiguration(discoveryRequest.getConfigurationParameters())); + uniqueId, new DefaultJupiterConfiguration(configurationParameters)); + // Handle the "multi-env" tests. List extensions = new ArrayList<>(); - AtomicBoolean envDiscovered = new AtomicBoolean(); + AtomicBoolean multiEnvDiscovered = new AtomicBoolean(); registry.stream() .forEach( ext -> { extensions.add(ext.getClass().getSimpleName()); - for (String envId : - ext.allEnvironmentIds(discoveryRequest.getConfigurationParameters())) { - envDiscovered.set(true); + for (String envId : ext.allEnvironmentIds(configurationParameters)) { + multiEnvDiscovered.set(true); UniqueId segment = uniqueId.append(ext.segmentType(), envId); MultiEnvTestDescriptor envRoot = new MultiEnvTestDescriptor(segment, envId); - multiEnvDescriptor.addChild(envRoot); + multiEnvRootDescriptor.addChild(envRoot); JupiterConfiguration envRootConfiguration = new CachingJupiterConfiguration( - new MultiEnvJupiterConfiguration( - discoveryRequest.getConfigurationParameters(), envId)); + new MultiEnvJupiterConfiguration(configurationParameters, envId)); JupiterEngineDescriptor discoverResult = new JupiterEngineDescriptor(segment, envRootConfiguration); new DiscoverySelectorResolver() @@ -116,18 +118,53 @@ public TestDescriptor discover(EngineDiscoveryRequest discoveryRequest, UniqueId for (TestDescriptor child : children) { // Note: this also removes the reference to parent from the child discoverResult.removeChild(child); - envRoot.addChild(child); + + // Must check whether the test class is a multi-env test, because discovery + // returns all test classes. + ClassBasedTestDescriptor classBased = (ClassBasedTestDescriptor) child; + boolean multi = registry().isMultiEnvClass(classBased); + if (multi) { + envRoot.addChild(child); + } + } + } + }); + + // Also execute all other tests via the MultiEnv test engine to get the "thread per + // test-class" behavior. + registry() + .probablyNotMultiEnv() + .forEach( + clazz -> { + JupiterConfiguration jupiterConfiguration = + new CachingJupiterConfiguration( + new DefaultJupiterConfiguration(configurationParameters)); + + JupiterEngineDescriptor discoverResult = + new JupiterEngineDescriptor(uniqueId, jupiterConfiguration); + new DiscoverySelectorResolver().resolveSelectors(discoveryRequest, discoverResult); + + List children = + new ArrayList<>(discoverResult.getChildren()); + for (TestDescriptor child : children) { + // Must check whether the test class is not a multi-env test here, as the + // `multiEnvNotDetected` contains some actual multi-env tests. + ClassBasedTestDescriptor classBased = (ClassBasedTestDescriptor) child; + boolean multi = registry().isMultiEnvClass(classBased); + if (!multi) { + discoverResult.removeChild(child); + multiEnvRootDescriptor.addChild(child); } } }); - if (!extensions.isEmpty() && !envDiscovered.get() && FAIL_ON_MISSING_ENVIRONMENTS) { + if (!extensions.isEmpty() && !multiEnvDiscovered.get() && FAIL_ON_MISSING_ENVIRONMENTS) { throw new IllegalStateException( String.format( "%s was enabled, but test extensions did not discover any environment IDs: %s", getClass().getSimpleName(), extensions)); } - return multiEnvDescriptor; + return multiEnvRootDescriptor; } catch (Exception e) { LOGGER.error("Failed to discover tests", e); throw new RuntimeException(e); diff --git a/testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestFilter.java b/testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestFilter.java index eb24474886b..5b91a4e34df 100644 --- a/testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestFilter.java +++ b/testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestFilter.java @@ -15,8 +15,6 @@ */ package org.projectnessie.junit.engine; -import static org.projectnessie.junit.engine.MultiEnvTestEngine.registry; - import java.util.Optional; import org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor; import org.junit.platform.engine.FilterResult; @@ -46,39 +44,27 @@ private Optional> classFor(TestDescriptor object) { return Optional.empty(); } - private FilterResult filter(Class testClass, UniqueId id) { - // Use the static extension data collected during the discovery phase. - // It is possible to reload extensions based of class objects from test descriptors, - // however that would add unnecessary overhead. - MultiEnvExtensionRegistry registry = registry(); - + /** + * This filter effectively routes all tests via the {@link MultiEnvTestEngine}, both actual + * multi-env tests but also non-multi-env tests to achieve the needed thread-per-test-class + * behavior. + * + *

"Thread-per-test-class behavior" is needed to prevent the class/class-loader leak via {@link + * ThreadLocal}s as described in #9441. + */ + private FilterResult filter(UniqueId id) { if (id.getEngineId().map("junit-jupiter"::equals).orElse(false)) { - if (registry.stream(testClass).findAny().isPresent()) { - return FilterResult.excluded("Excluding multi-env test from Jupiter Engine: " + id); - } else { - return FilterResult.included(null); - } + return FilterResult.excluded("Excluding multi-env test from Jupiter Engine: " + id); } else { - // check whether any of the extensions declared by the test recognize the version segment - boolean matched = - registry.stream(testClass) - .anyMatch( - ext -> - id.getSegments().stream() - .anyMatch(s -> ext.segmentType().equals(s.getType()))); - - if (matched) { - return FilterResult.included(null); - } else { - return FilterResult.excluded("Excluding unmatched multi-env test: " + id); - } + return FilterResult.included(null); } } @Override public FilterResult apply(TestDescriptor test) { return classFor(test) - .map(testClass -> filter(testClass, test.getUniqueId())) + .map(testClass -> filter(test.getUniqueId())) .orElseGet(() -> FilterResult.included(null)); // fallback for non-class descriptors } } diff --git a/testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/ThreadPerTestClassExecutionExecutorService.java b/testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/ThreadPerTestClassExecutionExecutorService.java new file mode 100644 index 00000000000..810e490d53e --- /dev/null +++ b/testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/ThreadPerTestClassExecutionExecutorService.java @@ -0,0 +1,117 @@ +/* + * Copyright (C) 2024 Dremio + * + * Licensed 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.projectnessie.junit.engine; + +import static java.util.concurrent.CompletableFuture.completedFuture; + +import java.lang.reflect.Field; +import java.util.List; +import java.util.concurrent.Future; +import java.util.concurrent.atomic.AtomicReference; +import org.junit.platform.engine.TestDescriptor; +import org.junit.platform.engine.UniqueId; +import org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutorService; + +/** + * Implements a JUnit test executor that provides thread-per-test-class behavior. + * + *

"Thread-per-test-class behavior" is needed to prevent the class/class-loader leak via {@link + * ThreadLocal}s as described in #9441. + */ +public class ThreadPerTestClassExecutionExecutorService implements HierarchicalTestExecutorService { + + private static final Class CLASS_NODE_TEST_TASK; + private static final Field FIELD_TEST_DESCRIPTOR; + + static { + try { + CLASS_NODE_TEST_TASK = + Class.forName("org.junit.platform.engine.support.hierarchical.NodeTestTask"); + FIELD_TEST_DESCRIPTOR = CLASS_NODE_TEST_TASK.getDeclaredField("testDescriptor"); + FIELD_TEST_DESCRIPTOR.setAccessible(true); + } catch (Exception e) { + throw new RuntimeException( + "ThreadPerExecutionExecutorService is probably not compatible with the current JUnit version", + e); + } + } + + protected TestDescriptor getTestDescriptor(TestTask testTask) { + if (!CLASS_NODE_TEST_TASK.isAssignableFrom(testTask.getClass())) { + throw new IllegalArgumentException( + testTask.getClass().getName() + " is not of type " + CLASS_NODE_TEST_TASK.getName()); + } + try { + return (TestDescriptor) FIELD_TEST_DESCRIPTOR.get(testTask); + } catch (IllegalAccessException e) { + throw new RuntimeException(e); + } + } + + public ThreadPerTestClassExecutionExecutorService() {} + + @Override + public Future submit(TestTask testTask) { + executeTask(testTask); + return completedFuture(null); + } + + @Override + public void invokeAll(List tasks) { + tasks.forEach(this::executeTask); + } + + protected void executeTask(TestTask testTask) { + TestDescriptor testDescriptor = getTestDescriptor(testTask); + UniqueId.Segment lastSegment = testDescriptor.getUniqueId().getLastSegment(); + String type = lastSegment.getType(); + if ("class".equals(type)) { + AtomicReference failure = new AtomicReference<>(); + Thread threadPerClass = + new Thread( + () -> { + try { + testTask.execute(); + } catch (Exception e) { + failure.set(e); + } + }, + "TEST THREAD FOR " + lastSegment.getValue()); + threadPerClass.setDaemon(true); + threadPerClass.start(); + try { + threadPerClass.join(); + } catch (InterruptedException e) { + // delegate a thread-interrupt + threadPerClass.interrupt(); + } + Exception ex = failure.get(); + if (ex instanceof RuntimeException) { + throw (RuntimeException) ex; + } else if (ex != null) { + throw new RuntimeException(ex); + } + } else { + testTask.execute(); + } + } + + @Override + public void close() { + // nothing to do + } +} diff --git a/testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/ThreadPerTestClassExecutionTestEngine.java b/testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/ThreadPerTestClassExecutionTestEngine.java new file mode 100644 index 00000000000..db68bf09328 --- /dev/null +++ b/testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/ThreadPerTestClassExecutionTestEngine.java @@ -0,0 +1,85 @@ +/* + * Copyright (C) 2024 Dremio + * + * Licensed 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.projectnessie.junit.engine; + +import java.util.Optional; +import org.junit.jupiter.engine.config.CachingJupiterConfiguration; +import org.junit.jupiter.engine.config.DefaultJupiterConfiguration; +import org.junit.jupiter.engine.config.JupiterConfiguration; +import org.junit.jupiter.engine.descriptor.JupiterEngineDescriptor; +import org.junit.jupiter.engine.discovery.DiscoverySelectorResolver; +import org.junit.jupiter.engine.execution.JupiterEngineExecutionContext; +import org.junit.jupiter.engine.support.JupiterThrowableCollectorFactory; +import org.junit.platform.engine.EngineDiscoveryRequest; +import org.junit.platform.engine.ExecutionRequest; +import org.junit.platform.engine.TestDescriptor; +import org.junit.platform.engine.UniqueId; +import org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine; +import org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutorService; +import org.junit.platform.engine.support.hierarchical.ThrowableCollector; + +public class ThreadPerTestClassExecutionTestEngine + extends HierarchicalTestEngine { + + @Override + public String getId() { + return JupiterEngineDescriptor.ENGINE_ID; + } + + /** Returns {@code org.junit.jupiter} as the group ID. */ + @Override + public Optional getGroupId() { + return Optional.of("org.junit.jupiter"); + } + + /** Returns {@code junit-jupiter-engine} as the artifact ID. */ + @Override + public Optional getArtifactId() { + return Optional.of("junit-jupiter-engine"); + } + + @Override + public TestDescriptor discover(EngineDiscoveryRequest discoveryRequest, UniqueId uniqueId) { + JupiterConfiguration configuration = + new CachingJupiterConfiguration( + new DefaultJupiterConfiguration(discoveryRequest.getConfigurationParameters())); + JupiterEngineDescriptor engineDescriptor = new JupiterEngineDescriptor(uniqueId, configuration); + new DiscoverySelectorResolver().resolveSelectors(discoveryRequest, engineDescriptor); + return engineDescriptor; + } + + @Override + protected HierarchicalTestExecutorService createExecutorService(ExecutionRequest request) { + return new ThreadPerTestClassExecutionExecutorService(); + } + + @Override + protected JupiterEngineExecutionContext createExecutionContext(ExecutionRequest request) { + return new JupiterEngineExecutionContext( + request.getEngineExecutionListener(), getJupiterConfiguration(request)); + } + + @Override + protected ThrowableCollector.Factory createThrowableCollectorFactory(ExecutionRequest request) { + return JupiterThrowableCollectorFactory::createThrowableCollector; + } + + private JupiterConfiguration getJupiterConfiguration(ExecutionRequest request) { + JupiterEngineDescriptor engineDescriptor = + (JupiterEngineDescriptor) request.getRootTestDescriptor(); + return engineDescriptor.getConfiguration(); + } +}