From 17729dee311911e314f25ac365d7e4ece7b7bce1 Mon Sep 17 00:00:00 2001 From: Chris Egerton Date: Mon, 19 Sep 2022 13:55:15 -0400 Subject: [PATCH 1/5] KAFKA-14244: Add guard against accidental calls to halt JVM during testing --- build.gradle | 9 +- .../kafka/test/DelegatingSecurityManager.java | 216 ++++++++++++++++++ .../kafka/test/junit/SystemExitExtension.java | 202 ++++++++++++++++ .../org.junit.jupiter.api.extension.Extension | 1 + 4 files changed, 426 insertions(+), 2 deletions(-) create mode 100644 core/src/test/java/kafka/test/DelegatingSecurityManager.java create mode 100644 core/src/test/java/kafka/test/junit/SystemExitExtension.java create mode 100644 core/src/test/resources/META-INF/services/org.junit.jupiter.api.extension.Extension diff --git a/build.gradle b/build.gradle index a61cd9f1d7432..d00745c9f5390 100644 --- a/build.gradle +++ b/build.gradle @@ -424,6 +424,8 @@ subprojects { maxHeapSize = defaultMaxHeapSize jvmArgs = defaultJvmArgs + systemProperty "junit.jupiter.extensions.autodetection.enabled", "true" + testLogging { events = userTestLoggingEvents ?: testLoggingEvents showStandardStreams = userShowStandardStreams ?: testShowStandardStreams @@ -456,6 +458,7 @@ subprojects { maxHeapSize = "2560m" jvmArgs = defaultJvmArgs + systemProperty "junit.jupiter.extensions.autodetection.enabled", "true" testLogging { events = userTestLoggingEvents ?: testLoggingEvents @@ -472,7 +475,7 @@ subprojects { useJUnitPlatform { includeTags "integration" includeTags "org.apache.kafka.test.IntegrationTest" - // Both engines are needed to run JUnit 4 tests alongside JUnit 5 tests. + // Both engines are needed to run JUnit 4 tests alongside JUnit 5 tests. // junit-vintage (JUnit 4) can be removed once the JUnit 4 migration is complete. includeEngines "junit-vintage", "junit-jupiter" } @@ -505,6 +508,8 @@ subprojects { maxHeapSize = defaultMaxHeapSize jvmArgs = defaultJvmArgs + systemProperty "junit.jupiter.extensions.autodetection.enabled", "true" + testLogging { events = userTestLoggingEvents ?: testLoggingEvents showStandardStreams = userShowStandardStreams ?: testShowStandardStreams @@ -520,7 +525,7 @@ subprojects { useJUnitPlatform { excludeTags "integration" excludeTags "org.apache.kafka.test.IntegrationTest" - // Both engines are needed to run JUnit 4 tests alongside JUnit 5 tests. + // Both engines are needed to run JUnit 4 tests alongside JUnit 5 tests. // junit-vintage (JUnit 4) can be removed once the JUnit 4 migration is complete. includeEngines "junit-vintage", "junit-jupiter" } diff --git a/core/src/test/java/kafka/test/DelegatingSecurityManager.java b/core/src/test/java/kafka/test/DelegatingSecurityManager.java new file mode 100644 index 0000000000000..069d130fa186a --- /dev/null +++ b/core/src/test/java/kafka/test/DelegatingSecurityManager.java @@ -0,0 +1,216 @@ +/* + * 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 kafka.test; + +import java.io.FileDescriptor; +import java.net.InetAddress; +import java.security.Permission; + +/** + * A {@link SecurityManager} implementation that accepts and wraps another {@link SecurityManager} instance. + * This class is not intended to be used directly, but as a superclass. Subclasses can selectively modify parts of the + * {@link SecurityManager} API without having to also implement boilerplate passthrough logic for the parts that + * they do not intend to modify. + */ +@SuppressWarnings({"deprecation", "removal"}) +public class DelegatingSecurityManager extends SecurityManager { + private final SecurityManager originalSecurityManager; + + /** + * Create a new {@link DelegatingSecurityManager}, which delegates all calls to {@link SecurityManager} API methods + * to the provided {@link SecurityManager} instance. + * @param originalSecurityManager the {@link SecurityManager} to delegate to; may be null + */ + public DelegatingSecurityManager(SecurityManager originalSecurityManager) { + this.originalSecurityManager = originalSecurityManager; + } + + @Override + public void checkExit(int status) { + if (originalSecurityManager != null) + originalSecurityManager.checkExit(status); + } + + @Override + public Object getSecurityContext() { + return (originalSecurityManager == null) ? super.getSecurityContext() + : originalSecurityManager.getSecurityContext(); + } + + @Override + public void checkPermission(Permission perm) { + if (originalSecurityManager != null) + originalSecurityManager.checkPermission(perm); + } + + @Override + public void checkPermission(Permission perm, Object context) { + if (originalSecurityManager != null) + originalSecurityManager.checkPermission(perm, context); + } + + @Override + public void checkCreateClassLoader() { + if (originalSecurityManager != null) + originalSecurityManager.checkCreateClassLoader(); + } + + @Override + public void checkAccess(Thread t) { + if (originalSecurityManager != null) + originalSecurityManager.checkAccess(t); + } + + @Override + public void checkAccess(ThreadGroup g) { + if (originalSecurityManager != null) + originalSecurityManager.checkAccess(g); + } + + @Override + public void checkExec(String cmd) { + if (originalSecurityManager != null) + originalSecurityManager.checkExec(cmd); + } + + @Override + public void checkLink(String lib) { + if (originalSecurityManager != null) + originalSecurityManager.checkLink(lib); + } + + @Override + public void checkRead(FileDescriptor fd) { + if (originalSecurityManager != null) + originalSecurityManager.checkRead(fd); + } + + @Override + public void checkRead(String file) { + if (originalSecurityManager != null) + originalSecurityManager.checkRead(file); + } + + @Override + public void checkRead(String file, Object context) { + if (originalSecurityManager != null) + originalSecurityManager.checkRead(file, context); + } + + @Override + public void checkWrite(FileDescriptor fd) { + if (originalSecurityManager != null) + originalSecurityManager.checkWrite(fd); + } + + @Override + public void checkWrite(String file) { + if (originalSecurityManager != null) + originalSecurityManager.checkWrite(file); + } + + @Override + public void checkDelete(String file) { + if (originalSecurityManager != null) + originalSecurityManager.checkDelete(file); + } + + @Override + public void checkConnect(String host, int port) { + if (originalSecurityManager != null) + originalSecurityManager.checkConnect(host, port); + } + + @Override + public void checkConnect(String host, int port, Object context) { + if (originalSecurityManager != null) + originalSecurityManager.checkConnect(host, port, context); + } + + @Override + public void checkListen(int port) { + if (originalSecurityManager != null) + originalSecurityManager.checkListen(port); + } + + @Override + public void checkAccept(String host, int port) { + if (originalSecurityManager != null) + originalSecurityManager.checkAccept(host, port); + } + + @Override + public void checkMulticast(InetAddress maddr) { + if (originalSecurityManager != null) + originalSecurityManager.checkMulticast(maddr); + } + + @Override + public void checkMulticast(InetAddress maddr, byte ttl) { + if (originalSecurityManager != null) + originalSecurityManager.checkMulticast(maddr, ttl); + } + + @Override + public void checkPropertiesAccess() { + if (originalSecurityManager != null) + originalSecurityManager.checkPropertiesAccess(); + } + + @Override + public void checkPropertyAccess(String key) { + if (originalSecurityManager != null) + originalSecurityManager.checkPropertyAccess(key); + } + + @Override + public void checkPrintJobAccess() { + if (originalSecurityManager != null) + originalSecurityManager.checkPrintJobAccess(); + } + + @Override + public void checkPackageAccess(String pkg) { + if (originalSecurityManager != null) + originalSecurityManager.checkPackageAccess(pkg); + } + + @Override + public void checkPackageDefinition(String pkg) { + if (originalSecurityManager != null) + originalSecurityManager.checkPackageDefinition(pkg); + } + + @Override + public void checkSetFactory() { + if (originalSecurityManager != null) + originalSecurityManager.checkSetFactory(); + } + + @Override + public void checkSecurityAccess(String target) { + if (originalSecurityManager != null) + originalSecurityManager.checkSecurityAccess(target); + } + + @Override + public ThreadGroup getThreadGroup() { + return (originalSecurityManager == null) ? super.getThreadGroup() + : originalSecurityManager.getThreadGroup(); + } +} diff --git a/core/src/test/java/kafka/test/junit/SystemExitExtension.java b/core/src/test/java/kafka/test/junit/SystemExitExtension.java new file mode 100644 index 0000000000000..cefc847a1766b --- /dev/null +++ b/core/src/test/java/kafka/test/junit/SystemExitExtension.java @@ -0,0 +1,202 @@ +/* + * 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 kafka.test.junit; + +import kafka.test.DelegatingSecurityManager; +import org.junit.jupiter.api.extension.AfterEachCallback; +import org.junit.jupiter.api.extension.BeforeEachCallback; +import org.junit.jupiter.api.extension.ExtensionContext; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.lang.reflect.Method; +import java.util.Collection; +import java.util.Map; +import java.util.Objects; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicReference; +import java.util.stream.Collectors; + +@SuppressWarnings({"deprecation", "removal"}) +public class SystemExitExtension implements BeforeEachCallback, AfterEachCallback { + + private static final Logger log = LoggerFactory.getLogger(SystemExitExtension.class); + private static final Map ACTIVE_TEST_CASES = new ConcurrentHashMap<>(); + private static final Map FINISHED_TEST_CASES = new ConcurrentHashMap<>(); + private static final InheritableThreadLocal CURRENT_TEST_INSTANCE = new InheritableThreadLocal<>(); + + private static volatile boolean canSetSecurityManager = true; + + public SystemExitExtension() { + log.debug("Instantiating system exit extension to guard against unintentional calls to terminate the JVM"); + } + + @Override + public void beforeEach(ExtensionContext context) { + maybeInstallSecurityManager(); + addActiveTestCase(context); + } + + @Override + public void afterEach(ExtensionContext context) { + finishActiveTestCase(context); + checkForLateExits(); + } + + private static void maybeInstallSecurityManager() { + if (!shouldInstallSecurityManager()) + return; + synchronized (SystemExitExtension.class) { + // Check again now that we've entered the synchronized block + if (!shouldInstallSecurityManager()) + return; + SecurityManager wrappedSecurityManager = new ExitCheckingSecurityManager(System.getSecurityManager()); + try { + System.setSecurityManager(wrappedSecurityManager); + } catch (Throwable t) { + // Some JVMs may not allow a security manager to be set; see https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/SecurityManager.html: + // "The Java run-time may also allow, but is not required to allow, the security manager to be set dynamically by invoking the setSecurityManager method." + // We don't want to fail the build if we can't set the security manager; better to log a warning and then continue normally + log.warn("Failed to set new security manager; tests will not be guarded against accidental JVM termination", t); + canSetSecurityManager = false; + } + } + } + + private static boolean shouldInstallSecurityManager() { + // If we've already tried and failed to set the security manager, we shouldn't try again + return canSetSecurityManager + // If we've already successfully registered the security manager, we don't need to do it again + && !(System.getSecurityManager() instanceof ExitCheckingSecurityManager); + } + + private static void addActiveTestCase(ExtensionContext context) { + Object testInstance = context.getRequiredTestInstance(); + CURRENT_TEST_INSTANCE.set(testInstance); + Method testMethod = context.getRequiredTestMethod(); + String testCaseDescription = testMethod.getDeclaringClass() + "::" + testMethod.getName(); + ACTIVE_TEST_CASES.put(testInstance, new TestCase(testCaseDescription)); + } + + private static void finishActiveTestCase(ExtensionContext context) { + // Any leaked threads from the test case will continue to see the value that they were instantiated with, + // but this thread should be reset since it may be reused for newer tests + CURRENT_TEST_INSTANCE.remove(); + + Object testInstance = context.getRequiredTestInstance(); + synchronized (SystemExitExtension.class) { + TestCase testCase = ACTIVE_TEST_CASES.remove(testInstance); + if (testCase == null) { + // Should never happen, but just in case... + return; + } + if (testCase.exit() != null) { + throw new AssertionError("System exit was invoked with status " + testCase.exit() + " during this test. " + + "Tests should never directly or indirectly invoke System::exit or System::halt and instead should use the Exit wrapper class, " + + "which can then be mocked during testing to avoid terminating the JVM when called"); + } else { + // Continue tracking tests that have completed without exiting as they may leak threads + // that try to exit later on + FINISHED_TEST_CASES.put(testInstance, testCase); + } + } + } + + private static void checkForLateExits() { + Collection lateExits; + synchronized (SystemExitExtension.class) { + lateExits = FINISHED_TEST_CASES.values().stream() + .filter(tc -> tc.exit() != null) + .collect(Collectors.toSet()); + FINISHED_TEST_CASES.values().removeAll(lateExits); + } + if (!lateExits.isEmpty()) { + throw new AssertionError( + "System exit was invoked by threads spawned for testing after those tests had completed; " + + "this test will fail in order to surface these illegal calls. The calls occurred in the following tests:\n" + + lateExits.stream().map(Object::toString).collect(Collectors.joining("\n")) + + "\nTests should never directly or indirectly invoke System::exit or System::halt and instead should use the Exit wrapper class, " + + "which can then be modified during testing to avoid terminating the JVM when called. " + + "Since the attempts to terminate the JVM originated from potentially-leaked threads, it may not be sufficient to " + + "use the Exit wrapper class, since its behavior must be reset at the end of each test, at which point other threads " + + "spawned during testing may attempt to use it. The offending test may have to be modified or disabled." + ); + } + } + + private static class TestCase { + private final String description; + private final AtomicReference exit; + + public TestCase(String description) { + this.description = Objects.requireNonNull(description); + this.exit = new AtomicReference<>(); + } + + public void recordExit(int status) { + exit.compareAndSet(null, status); + } + + public Integer exit() { + return exit.get(); + } + + @Override + public String toString() { + String result = description; + if (exit() != null) { + result += " (exited with status " + exit() + ")"; + } + return result; + } + } + + private static class ExitCheckingSecurityManager extends DelegatingSecurityManager { + + public ExitCheckingSecurityManager(SecurityManager originalSecurityManager) { + super(originalSecurityManager); + } + + @Override + public void checkExit(int status) { + Object testInstance = CURRENT_TEST_INSTANCE.get(); + if (testInstance == null) { + // We've probably finished testing and this is a "real" call to terminate the JVM + return; + } + + TestCase testCase; + synchronized (SystemExitExtension.class) { + testCase = ACTIVE_TEST_CASES.get(testInstance); + if (testCase == null) { + testCase = FINISHED_TEST_CASES.get(testInstance); + } + if (testCase == null) { + // In this case, it's possible that a single test has caused multiple attempts to terminate the JVM + // to take place, and we've already removed it from our collection of both active and finished test + // cases since we only need to report at most one attempt per test + return; + } + + testCase.recordExit(status); + } + + throw new AssertionError("Exit was invoked during test with status " + status); + } + } +} diff --git a/core/src/test/resources/META-INF/services/org.junit.jupiter.api.extension.Extension b/core/src/test/resources/META-INF/services/org.junit.jupiter.api.extension.Extension new file mode 100644 index 0000000000000..fdcf996a47279 --- /dev/null +++ b/core/src/test/resources/META-INF/services/org.junit.jupiter.api.extension.Extension @@ -0,0 +1 @@ +kafka.test.junit.SystemExitExtension \ No newline at end of file From 4d530130d191d22bfbfd442edf16a3bc574a2f34 Mon Sep 17 00:00:00 2001 From: Chris Egerton Date: Tue, 20 Sep 2022 13:14:57 -0400 Subject: [PATCH 2/5] Add license to service loader manifest file --- .../org.junit.jupiter.api.extension.Extension | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/core/src/test/resources/META-INF/services/org.junit.jupiter.api.extension.Extension b/core/src/test/resources/META-INF/services/org.junit.jupiter.api.extension.Extension index fdcf996a47279..c69f280d7bfc1 100644 --- a/core/src/test/resources/META-INF/services/org.junit.jupiter.api.extension.Extension +++ b/core/src/test/resources/META-INF/services/org.junit.jupiter.api.extension.Extension @@ -1 +1,15 @@ -kafka.test.junit.SystemExitExtension \ No newline at end of file +# 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. +kafka.test.junit.SystemExitExtension From 0d361d7ad5723bc6948decd9e06d7dbf87b9d389 Mon Sep 17 00:00:00 2001 From: Chris Egerton Date: Wed, 21 Sep 2022 12:28:41 -0400 Subject: [PATCH 3/5] Add support for Java 18 --- build.gradle | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/build.gradle b/build.gradle index d00745c9f5390..bfb857350fb83 100644 --- a/build.gradle +++ b/build.gradle @@ -425,6 +425,9 @@ subprojects { jvmArgs = defaultJvmArgs systemProperty "junit.jupiter.extensions.autodetection.enabled", "true" + if (JavaVersion.current().isJava12Compatible()) { + systemProperty "java.security.manager", "allow" + } testLogging { events = userTestLoggingEvents ?: testLoggingEvents @@ -459,6 +462,9 @@ subprojects { jvmArgs = defaultJvmArgs systemProperty "junit.jupiter.extensions.autodetection.enabled", "true" + if (JavaVersion.current().isJava12Compatible()) { + systemProperty "java.security.manager", "allow" + } testLogging { events = userTestLoggingEvents ?: testLoggingEvents @@ -509,6 +515,9 @@ subprojects { jvmArgs = defaultJvmArgs systemProperty "junit.jupiter.extensions.autodetection.enabled", "true" + if (JavaVersion.current().isJava12Compatible()) { + systemProperty "java.security.manager", "allow" + } testLogging { events = userTestLoggingEvents ?: testLoggingEvents From 54b663dbbb7d90eb18dc62bb4fcfff411a00aca5 Mon Sep 17 00:00:00 2001 From: Chris Egerton Date: Fri, 23 Sep 2022 16:54:43 -0400 Subject: [PATCH 4/5] Add tests --- .../kafka/test/junit/SystemExitExtension.java | 63 +++++++- .../test/junit/SystemExitExtensionTest.scala | 135 ++++++++++++++++++ 2 files changed, 196 insertions(+), 2 deletions(-) create mode 100644 core/src/test/scala/kafka/test/junit/SystemExitExtensionTest.scala diff --git a/core/src/test/java/kafka/test/junit/SystemExitExtension.java b/core/src/test/java/kafka/test/junit/SystemExitExtension.java index cefc847a1766b..fc32643fe8526 100644 --- a/core/src/test/java/kafka/test/junit/SystemExitExtension.java +++ b/core/src/test/java/kafka/test/junit/SystemExitExtension.java @@ -32,6 +32,9 @@ import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; + @SuppressWarnings({"deprecation", "removal"}) public class SystemExitExtension implements BeforeEachCallback, AfterEachCallback { @@ -58,6 +61,48 @@ public void afterEach(ExtensionContext context) { checkForLateExits(); } + /** + * For testing the extension; should not be used by any other tests + */ + static boolean securityManagerInstalled() { + return System.getSecurityManager() instanceof ExitCheckingSecurityManager; + } + + /** + * For testing the extension; should not be used by any other tests + */ + static void allowExitFromCurrentTest() { + Object testInstance = CURRENT_TEST_INSTANCE.get(); + assertNotNull(testInstance); + TestCase testCase = ACTIVE_TEST_CASES.get(testInstance); + assertNotNull(testCase); + testCase.allowExit(true); + } + + /** + * For testing the extension; should not be used by any other tests + */ + static void assertExitCalledFromCurrentTest(int expectedStatus) { + Object testInstance = CURRENT_TEST_INSTANCE.get(); + assertNotNull(testInstance); + TestCase testCase = ACTIVE_TEST_CASES.remove(testInstance); + assertExitFromTest(testCase, expectedStatus); + } + + /** + * For testing the extension; should not be used by any other tests + */ + static void assertExitFromPriorTest(Object testInstance, int expectedStatus) { + TestCase testCase = FINISHED_TEST_CASES.remove(testInstance); + assertExitFromTest(testCase, expectedStatus); + } + + private static void assertExitFromTest(TestCase testCase, int expectedStatus) { + assertNotNull(testCase); + Integer actualStatus = testCase.exit(); + assertEquals(Integer.valueOf(expectedStatus), actualStatus); + } + private static void maybeInstallSecurityManager() { if (!shouldInstallSecurityManager()) return; @@ -82,7 +127,7 @@ private static boolean shouldInstallSecurityManager() { // If we've already tried and failed to set the security manager, we shouldn't try again return canSetSecurityManager // If we've already successfully registered the security manager, we don't need to do it again - && !(System.getSecurityManager() instanceof ExitCheckingSecurityManager); + && !securityManagerInstalled(); } private static void addActiveTestCase(ExtensionContext context) { @@ -105,7 +150,7 @@ private static void finishActiveTestCase(ExtensionContext context) { // Should never happen, but just in case... return; } - if (testCase.exit() != null) { + if (testCase.exit() != null && !testCase.allowExit()) { throw new AssertionError("System exit was invoked with status " + testCase.exit() + " during this test. " + "Tests should never directly or indirectly invoke System::exit or System::halt and instead should use the Exit wrapper class, " + "which can then be mocked during testing to avoid terminating the JVM when called"); @@ -122,6 +167,7 @@ private static void checkForLateExits() { synchronized (SystemExitExtension.class) { lateExits = FINISHED_TEST_CASES.values().stream() .filter(tc -> tc.exit() != null) + .filter(tc -> !tc.allowExit()) .collect(Collectors.toSet()); FINISHED_TEST_CASES.values().removeAll(lateExits); } @@ -140,10 +186,13 @@ private static void checkForLateExits() { } private static class TestCase { + + private volatile boolean allowExit; private final String description; private final AtomicReference exit; public TestCase(String description) { + this.allowExit = false; this.description = Objects.requireNonNull(description); this.exit = new AtomicReference<>(); } @@ -156,6 +205,16 @@ public Integer exit() { return exit.get(); } + // For testing only { + public boolean allowExit() { + return allowExit; + } + + // For testing only + public void allowExit(boolean allowExit) { + this.allowExit = allowExit; + } + @Override public String toString() { String result = description; diff --git a/core/src/test/scala/kafka/test/junit/SystemExitExtensionTest.scala b/core/src/test/scala/kafka/test/junit/SystemExitExtensionTest.scala new file mode 100644 index 0000000000000..58a606b40c81e --- /dev/null +++ b/core/src/test/scala/kafka/test/junit/SystemExitExtensionTest.scala @@ -0,0 +1,135 @@ +/* + * 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 kafka.test.junit + +import org.junit.jupiter.api.Assertions.{assertInstanceOf, assertNotNull, assertThrows, assertTrue} +import org.junit.jupiter.api.{MethodOrderer, Order, Test, TestMethodOrder} + +import java.util.concurrent.{CountDownLatch, TimeUnit} +import java.util.concurrent.atomic.AtomicReference + +object SystemExitExtensionTest { + private val lateExitStatus = 923 + private val lateExitOrigin = new AtomicReference[Object] + + @volatile private var lateExitReady: CountDownLatch = _ + @volatile private var lateExitInvoked: CountDownLatch = _ +} + +@TestMethodOrder(classOf[MethodOrderer.OrderAnnotation]) +class SystemExitExtensionTest { + + @Test + @Order(1) + def testSystemExitInvokedOnMainTestThread(): Unit = { + if (!SystemExitExtension.securityManagerInstalled) + return + + SystemExitExtension.allowExitFromCurrentTest() + + val exitStatus = 618 + // The caller should know that the attempt to terminate the JVM has failed + assertThrows(classOf[AssertionError], () => System.exit(exitStatus)) + // The extension should track that exit has been called in this test as well + SystemExitExtension.assertExitCalledFromCurrentTest(exitStatus) + } + + @Test + @Order(2) + def testSystemExitInvokedOnSeparateTestThread(): Unit = { + if (!SystemExitExtension.securityManagerInstalled) + return + + SystemExitExtension.allowExitFromCurrentTest() + + val exitStatus = 815 + val exitInvoked = new CountDownLatch(1) + val exitException = new AtomicReference[Throwable]() + new Thread(() => { + try { + System.exit(exitStatus) + } catch { + case t: Throwable => exitException.set(t) + } finally { + exitInvoked.countDown() + } + }).start() + + awaitLatch( + exitInvoked, + "Thread created during this test did not complete execution in time" + ) + // Wait for our separate thread to complete + exitInvoked.await(10, TimeUnit.SECONDS) + + // The caller should know that the attempt to terminate the JVM has failed + assertNotNull(exitException.get) + assertInstanceOf(classOf[AssertionError], exitException.get) + + // The extension should track that exit has been called in this test as well + SystemExitExtension.assertExitCalledFromCurrentTest(exitStatus) + } + + @Test + @Order(3) + // This test doesn't actually make any assertions; all it does is "leak" a thread that + // then invokes System::exit later on while a different test is running, so that that + // other test can ensure that the call was properly handled and attributed to this one + def spawnLateExitingTest(): Unit = { + if (!SystemExitExtension.securityManagerInstalled) + return + + SystemExitExtension.allowExitFromCurrentTest() + + SystemExitExtensionTest.lateExitOrigin.set(this) + SystemExitExtensionTest.lateExitReady = new CountDownLatch(1) + SystemExitExtensionTest.lateExitInvoked = new CountDownLatch(1) + + new Thread(() => { + // Await this latch indefinitely + SystemExitExtensionTest.lateExitReady.await() + try { + System.exit(SystemExitExtensionTest.lateExitStatus) + } finally { + SystemExitExtensionTest.lateExitInvoked.countDown() + } + }).start() + } + + @Test + @Order(4) + // This test ensures that the "leaked" thread from the above test is properly + // handled and attributed to that test + def testSystemExitInvokedOnLeakedTestThread(): Unit = { + if (!SystemExitExtension.securityManagerInstalled) + return + + assertNotNull(SystemExitExtensionTest.lateExitOrigin.get()) + SystemExitExtensionTest.lateExitReady.countDown() + awaitLatch( + SystemExitExtensionTest.lateExitInvoked, + "Leaked thread from prior test did not complete execution in time" + ) + + SystemExitExtension.assertExitFromPriorTest(SystemExitExtensionTest.lateExitOrigin.get(), SystemExitExtensionTest.lateExitStatus) + } + + private def awaitLatch(latch: CountDownLatch, assertionMessage: String): Unit = { + assertTrue(latch.await(10, TimeUnit.SECONDS), assertionMessage) + } + +} From 91cd8bc92b1b09ef4767bd49856361706ab12afe Mon Sep 17 00:00:00 2001 From: Chris Egerton Date: Tue, 1 Nov 2022 15:57:10 -0400 Subject: [PATCH 5/5] Remove custom SecurityManager --- build.gradle | 9 - .../org/apache/kafka/common/utils/Exit.java | 84 ++++++- .../kafka/test/DelegatingSecurityManager.java | 216 ------------------ .../kafka/test/junit/SystemExitExtension.java | 82 ++----- .../test/junit/SystemExitExtensionTest.scala | 19 +- 5 files changed, 100 insertions(+), 310 deletions(-) delete mode 100644 core/src/test/java/kafka/test/DelegatingSecurityManager.java diff --git a/build.gradle b/build.gradle index bfb857350fb83..d00745c9f5390 100644 --- a/build.gradle +++ b/build.gradle @@ -425,9 +425,6 @@ subprojects { jvmArgs = defaultJvmArgs systemProperty "junit.jupiter.extensions.autodetection.enabled", "true" - if (JavaVersion.current().isJava12Compatible()) { - systemProperty "java.security.manager", "allow" - } testLogging { events = userTestLoggingEvents ?: testLoggingEvents @@ -462,9 +459,6 @@ subprojects { jvmArgs = defaultJvmArgs systemProperty "junit.jupiter.extensions.autodetection.enabled", "true" - if (JavaVersion.current().isJava12Compatible()) { - systemProperty "java.security.manager", "allow" - } testLogging { events = userTestLoggingEvents ?: testLoggingEvents @@ -515,9 +509,6 @@ subprojects { jvmArgs = defaultJvmArgs systemProperty "junit.jupiter.extensions.autodetection.enabled", "true" - if (JavaVersion.current().isJava12Compatible()) { - systemProperty "java.security.manager", "allow" - } testLogging { events = userTestLoggingEvents ?: testLoggingEvents diff --git a/clients/src/main/java/org/apache/kafka/common/utils/Exit.java b/clients/src/main/java/org/apache/kafka/common/utils/Exit.java index 20a7b2035ca16..827ef86c37917 100644 --- a/clients/src/main/java/org/apache/kafka/common/utils/Exit.java +++ b/clients/src/main/java/org/apache/kafka/common/utils/Exit.java @@ -54,8 +54,25 @@ public void addShutdownHook(String name, Runnable runnable) { } }; - private volatile static Procedure exitProcedure = DEFAULT_EXIT_PROCEDURE; - private volatile static Procedure haltProcedure = DEFAULT_HALT_PROCEDURE; + // The procedures to use if no custom procedure has been installed via setExitProcedure/setHaltProcedure + private volatile static Procedure fallbackExitProcedure = DEFAULT_EXIT_PROCEDURE; + private volatile static Procedure fallbackHaltProcedure = DEFAULT_HALT_PROCEDURE; + + // Use InheritableThreadLocal so that all threads use the same custom procedure(s), but + // once new test cases are started, leaked threads from prior tests don't have those custom procedures + // overwritten by the new cases, and don't accidentally invoke the custom procedures for the new cases + private static final InheritableThreadLocal EXIT_PROCEDURE = new InheritableThreadLocal() { + @Override + protected ProcedureWithFallback initialValue() { + return ProcedureWithFallback.forExit(); + } + }; + private static final InheritableThreadLocal HALT_PROCEDURE = new InheritableThreadLocal() { + @Override + protected ProcedureWithFallback initialValue() { + return ProcedureWithFallback.forHalt(); + } + }; private volatile static ShutdownHookAdder shutdownHookAdder = DEFAULT_SHUTDOWN_HOOK_ADDER; public static void exit(int statusCode) { @@ -63,7 +80,7 @@ public static void exit(int statusCode) { } public static void exit(int statusCode, String message) { - exitProcedure.execute(statusCode, message); + EXIT_PROCEDURE.get().procedure().execute(statusCode, message); } public static void halt(int statusCode) { @@ -71,19 +88,29 @@ public static void halt(int statusCode) { } public static void halt(int statusCode, String message) { - haltProcedure.execute(statusCode, message); + HALT_PROCEDURE.get().procedure().execute(statusCode, message); } public static void addShutdownHook(String name, Runnable runnable) { shutdownHookAdder.addShutdownHook(name, runnable); } + public static void setFallbackExitProcedure(Procedure procedure) { + fallbackExitProcedure = procedure; + EXIT_PROCEDURE.get().setFallback(procedure); + } + + public static void setFallbackHaltProcedure(Procedure procedure) { + fallbackHaltProcedure = procedure; + HALT_PROCEDURE.get().setFallback(procedure); + } + public static void setExitProcedure(Procedure procedure) { - exitProcedure = procedure; + EXIT_PROCEDURE.get().setCustom(procedure); } public static void setHaltProcedure(Procedure procedure) { - haltProcedure = procedure; + HALT_PROCEDURE.get().setCustom(procedure); } public static void setShutdownHookAdder(ShutdownHookAdder shutdownHookAdder) { @@ -91,14 +118,55 @@ public static void setShutdownHookAdder(ShutdownHookAdder shutdownHookAdder) { } public static void resetExitProcedure() { - exitProcedure = DEFAULT_EXIT_PROCEDURE; + EXIT_PROCEDURE.get().useFallback(); + EXIT_PROCEDURE.set(ProcedureWithFallback.forExit()); } public static void resetHaltProcedure() { - haltProcedure = DEFAULT_HALT_PROCEDURE; + HALT_PROCEDURE.get().useFallback(); + HALT_PROCEDURE.set(ProcedureWithFallback.forHalt()); } public static void resetShutdownHookAdder() { shutdownHookAdder = DEFAULT_SHUTDOWN_HOOK_ADDER; } + + private static class ProcedureWithFallback { + private volatile Procedure fallback; + private volatile Procedure custom; + private volatile boolean useCustom; + + public static ProcedureWithFallback forExit() { + return new ProcedureWithFallback(fallbackExitProcedure); + } + + public static ProcedureWithFallback forHalt() { + return new ProcedureWithFallback(fallbackHaltProcedure); + } + + private ProcedureWithFallback(Procedure fallbackProcedure) { + this.fallback = fallbackProcedure; + this.custom = null; + this.useCustom = true; + } + + public void setCustom(Procedure procedure) { + this.custom = procedure; + } + + public void setFallback(Procedure procedure) { + this.fallback = procedure; + } + + // Disable any custom procedures and force the fallback to be used; should be invoked + // once a test case has completed, in order to prevent leaked threads from that test case + // from calling the custom procedure + public void useFallback() { + useCustom = false; + } + + public Procedure procedure() { + return custom != null && useCustom ? custom : fallback; + } + } } diff --git a/core/src/test/java/kafka/test/DelegatingSecurityManager.java b/core/src/test/java/kafka/test/DelegatingSecurityManager.java deleted file mode 100644 index 069d130fa186a..0000000000000 --- a/core/src/test/java/kafka/test/DelegatingSecurityManager.java +++ /dev/null @@ -1,216 +0,0 @@ -/* - * 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 kafka.test; - -import java.io.FileDescriptor; -import java.net.InetAddress; -import java.security.Permission; - -/** - * A {@link SecurityManager} implementation that accepts and wraps another {@link SecurityManager} instance. - * This class is not intended to be used directly, but as a superclass. Subclasses can selectively modify parts of the - * {@link SecurityManager} API without having to also implement boilerplate passthrough logic for the parts that - * they do not intend to modify. - */ -@SuppressWarnings({"deprecation", "removal"}) -public class DelegatingSecurityManager extends SecurityManager { - private final SecurityManager originalSecurityManager; - - /** - * Create a new {@link DelegatingSecurityManager}, which delegates all calls to {@link SecurityManager} API methods - * to the provided {@link SecurityManager} instance. - * @param originalSecurityManager the {@link SecurityManager} to delegate to; may be null - */ - public DelegatingSecurityManager(SecurityManager originalSecurityManager) { - this.originalSecurityManager = originalSecurityManager; - } - - @Override - public void checkExit(int status) { - if (originalSecurityManager != null) - originalSecurityManager.checkExit(status); - } - - @Override - public Object getSecurityContext() { - return (originalSecurityManager == null) ? super.getSecurityContext() - : originalSecurityManager.getSecurityContext(); - } - - @Override - public void checkPermission(Permission perm) { - if (originalSecurityManager != null) - originalSecurityManager.checkPermission(perm); - } - - @Override - public void checkPermission(Permission perm, Object context) { - if (originalSecurityManager != null) - originalSecurityManager.checkPermission(perm, context); - } - - @Override - public void checkCreateClassLoader() { - if (originalSecurityManager != null) - originalSecurityManager.checkCreateClassLoader(); - } - - @Override - public void checkAccess(Thread t) { - if (originalSecurityManager != null) - originalSecurityManager.checkAccess(t); - } - - @Override - public void checkAccess(ThreadGroup g) { - if (originalSecurityManager != null) - originalSecurityManager.checkAccess(g); - } - - @Override - public void checkExec(String cmd) { - if (originalSecurityManager != null) - originalSecurityManager.checkExec(cmd); - } - - @Override - public void checkLink(String lib) { - if (originalSecurityManager != null) - originalSecurityManager.checkLink(lib); - } - - @Override - public void checkRead(FileDescriptor fd) { - if (originalSecurityManager != null) - originalSecurityManager.checkRead(fd); - } - - @Override - public void checkRead(String file) { - if (originalSecurityManager != null) - originalSecurityManager.checkRead(file); - } - - @Override - public void checkRead(String file, Object context) { - if (originalSecurityManager != null) - originalSecurityManager.checkRead(file, context); - } - - @Override - public void checkWrite(FileDescriptor fd) { - if (originalSecurityManager != null) - originalSecurityManager.checkWrite(fd); - } - - @Override - public void checkWrite(String file) { - if (originalSecurityManager != null) - originalSecurityManager.checkWrite(file); - } - - @Override - public void checkDelete(String file) { - if (originalSecurityManager != null) - originalSecurityManager.checkDelete(file); - } - - @Override - public void checkConnect(String host, int port) { - if (originalSecurityManager != null) - originalSecurityManager.checkConnect(host, port); - } - - @Override - public void checkConnect(String host, int port, Object context) { - if (originalSecurityManager != null) - originalSecurityManager.checkConnect(host, port, context); - } - - @Override - public void checkListen(int port) { - if (originalSecurityManager != null) - originalSecurityManager.checkListen(port); - } - - @Override - public void checkAccept(String host, int port) { - if (originalSecurityManager != null) - originalSecurityManager.checkAccept(host, port); - } - - @Override - public void checkMulticast(InetAddress maddr) { - if (originalSecurityManager != null) - originalSecurityManager.checkMulticast(maddr); - } - - @Override - public void checkMulticast(InetAddress maddr, byte ttl) { - if (originalSecurityManager != null) - originalSecurityManager.checkMulticast(maddr, ttl); - } - - @Override - public void checkPropertiesAccess() { - if (originalSecurityManager != null) - originalSecurityManager.checkPropertiesAccess(); - } - - @Override - public void checkPropertyAccess(String key) { - if (originalSecurityManager != null) - originalSecurityManager.checkPropertyAccess(key); - } - - @Override - public void checkPrintJobAccess() { - if (originalSecurityManager != null) - originalSecurityManager.checkPrintJobAccess(); - } - - @Override - public void checkPackageAccess(String pkg) { - if (originalSecurityManager != null) - originalSecurityManager.checkPackageAccess(pkg); - } - - @Override - public void checkPackageDefinition(String pkg) { - if (originalSecurityManager != null) - originalSecurityManager.checkPackageDefinition(pkg); - } - - @Override - public void checkSetFactory() { - if (originalSecurityManager != null) - originalSecurityManager.checkSetFactory(); - } - - @Override - public void checkSecurityAccess(String target) { - if (originalSecurityManager != null) - originalSecurityManager.checkSecurityAccess(target); - } - - @Override - public ThreadGroup getThreadGroup() { - return (originalSecurityManager == null) ? super.getThreadGroup() - : originalSecurityManager.getThreadGroup(); - } -} diff --git a/core/src/test/java/kafka/test/junit/SystemExitExtension.java b/core/src/test/java/kafka/test/junit/SystemExitExtension.java index fc32643fe8526..85199799cc739 100644 --- a/core/src/test/java/kafka/test/junit/SystemExitExtension.java +++ b/core/src/test/java/kafka/test/junit/SystemExitExtension.java @@ -17,7 +17,7 @@ package kafka.test.junit; -import kafka.test.DelegatingSecurityManager; +import org.apache.kafka.common.utils.Exit; import org.junit.jupiter.api.extension.AfterEachCallback; import org.junit.jupiter.api.extension.BeforeEachCallback; import org.junit.jupiter.api.extension.ExtensionContext; @@ -35,7 +35,6 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; -@SuppressWarnings({"deprecation", "removal"}) public class SystemExitExtension implements BeforeEachCallback, AfterEachCallback { private static final Logger log = LoggerFactory.getLogger(SystemExitExtension.class); @@ -43,15 +42,12 @@ public class SystemExitExtension implements BeforeEachCallback, AfterEachCallbac private static final Map FINISHED_TEST_CASES = new ConcurrentHashMap<>(); private static final InheritableThreadLocal CURRENT_TEST_INSTANCE = new InheritableThreadLocal<>(); - private static volatile boolean canSetSecurityManager = true; - public SystemExitExtension() { log.debug("Instantiating system exit extension to guard against unintentional calls to terminate the JVM"); } @Override public void beforeEach(ExtensionContext context) { - maybeInstallSecurityManager(); addActiveTestCase(context); } @@ -61,13 +57,6 @@ public void afterEach(ExtensionContext context) { checkForLateExits(); } - /** - * For testing the extension; should not be used by any other tests - */ - static boolean securityManagerInstalled() { - return System.getSecurityManager() instanceof ExitCheckingSecurityManager; - } - /** * For testing the extension; should not be used by any other tests */ @@ -103,42 +92,19 @@ private static void assertExitFromTest(TestCase testCase, int expectedStatus) { assertEquals(Integer.valueOf(expectedStatus), actualStatus); } - private static void maybeInstallSecurityManager() { - if (!shouldInstallSecurityManager()) - return; - synchronized (SystemExitExtension.class) { - // Check again now that we've entered the synchronized block - if (!shouldInstallSecurityManager()) - return; - SecurityManager wrappedSecurityManager = new ExitCheckingSecurityManager(System.getSecurityManager()); - try { - System.setSecurityManager(wrappedSecurityManager); - } catch (Throwable t) { - // Some JVMs may not allow a security manager to be set; see https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/SecurityManager.html: - // "The Java run-time may also allow, but is not required to allow, the security manager to be set dynamically by invoking the setSecurityManager method." - // We don't want to fail the build if we can't set the security manager; better to log a warning and then continue normally - log.warn("Failed to set new security manager; tests will not be guarded against accidental JVM termination", t); - canSetSecurityManager = false; - } - } - } - - private static boolean shouldInstallSecurityManager() { - // If we've already tried and failed to set the security manager, we shouldn't try again - return canSetSecurityManager - // If we've already successfully registered the security manager, we don't need to do it again - && !securityManagerInstalled(); - } - private static void addActiveTestCase(ExtensionContext context) { Object testInstance = context.getRequiredTestInstance(); CURRENT_TEST_INSTANCE.set(testInstance); Method testMethod = context.getRequiredTestMethod(); String testCaseDescription = testMethod.getDeclaringClass() + "::" + testMethod.getName(); ACTIVE_TEST_CASES.put(testInstance, new TestCase(testCaseDescription)); + Exit.setFallbackExitProcedure(exitHandler(testInstance)); + Exit.setFallbackHaltProcedure(exitHandler(testInstance)); } private static void finishActiveTestCase(ExtensionContext context) { + Exit.resetExitProcedure(); + Exit.resetHaltProcedure(); // Any leaked threads from the test case will continue to see the value that they were instantiated with, // but this thread should be reset since it may be reused for newer tests CURRENT_TEST_INSTANCE.remove(); @@ -151,9 +117,9 @@ private static void finishActiveTestCase(ExtensionContext context) { return; } if (testCase.exit() != null && !testCase.allowExit()) { - throw new AssertionError("System exit was invoked with status " + testCase.exit() + " during this test. " - + "Tests should never directly or indirectly invoke System::exit or System::halt and instead should use the Exit wrapper class, " - + "which can then be mocked during testing to avoid terminating the JVM when called"); + throw new AssertionError("Exit/halt was invoked with status " + testCase.exit() + " during this test. " + + "This test should either be modified to install a custom exit/halt procedure in the Exit class, or " + + "if no calls to Exit::exit or Exit::halt are expected, fixed to prevent these calls from taking place."); } else { // Continue tracking tests that have completed without exiting as they may leak threads // that try to exit later on @@ -173,11 +139,9 @@ private static void checkForLateExits() { } if (!lateExits.isEmpty()) { throw new AssertionError( - "System exit was invoked by threads spawned for testing after those tests had completed; " + "Exit/halt was invoked by threads spawned for testing after those tests had completed; " + "this test will fail in order to surface these illegal calls. The calls occurred in the following tests:\n" + lateExits.stream().map(Object::toString).collect(Collectors.joining("\n")) - + "\nTests should never directly or indirectly invoke System::exit or System::halt and instead should use the Exit wrapper class, " - + "which can then be modified during testing to avoid terminating the JVM when called. " + "Since the attempts to terminate the JVM originated from potentially-leaked threads, it may not be sufficient to " + "use the Exit wrapper class, since its behavior must be reset at the end of each test, at which point other threads " + "spawned during testing may attempt to use it. The offending test may have to be modified or disabled." @@ -225,20 +189,8 @@ public String toString() { } } - private static class ExitCheckingSecurityManager extends DelegatingSecurityManager { - - public ExitCheckingSecurityManager(SecurityManager originalSecurityManager) { - super(originalSecurityManager); - } - - @Override - public void checkExit(int status) { - Object testInstance = CURRENT_TEST_INSTANCE.get(); - if (testInstance == null) { - // We've probably finished testing and this is a "real" call to terminate the JVM - return; - } - + private static org.apache.kafka.common.utils.Exit.Procedure exitHandler(Object testInstance) { + return (statusCode, message) -> { TestCase testCase; synchronized (SystemExitExtension.class) { testCase = ACTIVE_TEST_CASES.get(testInstance); @@ -252,10 +204,16 @@ public void checkExit(int status) { return; } - testCase.recordExit(status); + testCase.recordExit(statusCode); } - throw new AssertionError("Exit was invoked during test with status " + status); - } + String errorMessage = "Exit or halt was invoked during test with status " + statusCode; + if (message != null) { + errorMessage += " and message '" + message + "'"; + } + + throw new AssertionError(errorMessage); + }; } + } diff --git a/core/src/test/scala/kafka/test/junit/SystemExitExtensionTest.scala b/core/src/test/scala/kafka/test/junit/SystemExitExtensionTest.scala index 58a606b40c81e..3f9994064c415 100644 --- a/core/src/test/scala/kafka/test/junit/SystemExitExtensionTest.scala +++ b/core/src/test/scala/kafka/test/junit/SystemExitExtensionTest.scala @@ -16,6 +16,7 @@ */ package kafka.test.junit +import kafka.utils.Exit import org.junit.jupiter.api.Assertions.{assertInstanceOf, assertNotNull, assertThrows, assertTrue} import org.junit.jupiter.api.{MethodOrderer, Order, Test, TestMethodOrder} @@ -36,14 +37,11 @@ class SystemExitExtensionTest { @Test @Order(1) def testSystemExitInvokedOnMainTestThread(): Unit = { - if (!SystemExitExtension.securityManagerInstalled) - return - SystemExitExtension.allowExitFromCurrentTest() val exitStatus = 618 // The caller should know that the attempt to terminate the JVM has failed - assertThrows(classOf[AssertionError], () => System.exit(exitStatus)) + assertThrows(classOf[AssertionError], () => Exit.exit(exitStatus)) // The extension should track that exit has been called in this test as well SystemExitExtension.assertExitCalledFromCurrentTest(exitStatus) } @@ -51,9 +49,6 @@ class SystemExitExtensionTest { @Test @Order(2) def testSystemExitInvokedOnSeparateTestThread(): Unit = { - if (!SystemExitExtension.securityManagerInstalled) - return - SystemExitExtension.allowExitFromCurrentTest() val exitStatus = 815 @@ -61,7 +56,7 @@ class SystemExitExtensionTest { val exitException = new AtomicReference[Throwable]() new Thread(() => { try { - System.exit(exitStatus) + Exit.exit(exitStatus) } catch { case t: Throwable => exitException.set(t) } finally { @@ -90,9 +85,6 @@ class SystemExitExtensionTest { // then invokes System::exit later on while a different test is running, so that that // other test can ensure that the call was properly handled and attributed to this one def spawnLateExitingTest(): Unit = { - if (!SystemExitExtension.securityManagerInstalled) - return - SystemExitExtension.allowExitFromCurrentTest() SystemExitExtensionTest.lateExitOrigin.set(this) @@ -103,7 +95,7 @@ class SystemExitExtensionTest { // Await this latch indefinitely SystemExitExtensionTest.lateExitReady.await() try { - System.exit(SystemExitExtensionTest.lateExitStatus) + Exit.exit(SystemExitExtensionTest.lateExitStatus) } finally { SystemExitExtensionTest.lateExitInvoked.countDown() } @@ -115,9 +107,6 @@ class SystemExitExtensionTest { // This test ensures that the "leaked" thread from the above test is properly // handled and attributed to that test def testSystemExitInvokedOnLeakedTestThread(): Unit = { - if (!SystemExitExtension.securityManagerInstalled) - return - assertNotNull(SystemExitExtensionTest.lateExitOrigin.get()) SystemExitExtensionTest.lateExitReady.countDown() awaitLatch(