This is an automated email from the ASF dual-hosted git repository. ggregory pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit 7847b82a874f279cf710084b3e32e18679f2e4e3 Author: Gary Gregory <[email protected]> AuthorDate: Wed Feb 24 19:33:01 2021 -0500 [LOG4J2-3026] WatchManager does not stop its ConfigurationScheduler thereby leaking a thread. --- .../logging/log4j/core/util/WatchManager.java | 10 +++++- .../logging/log4j/core/util/WatchHttpTest.java | 5 +-- .../logging/log4j/core/util/WatchManagerTest.java | 40 +++++++++++----------- src/changes/changes.xml | 3 ++ 4 files changed, 35 insertions(+), 23 deletions(-) diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/WatchManager.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/WatchManager.java index f58f67b..89d129b 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/WatchManager.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/WatchManager.java @@ -29,6 +29,7 @@ import java.util.Date; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.ServiceLoader; import java.util.UUID; import java.util.concurrent.ConcurrentHashMap; @@ -132,7 +133,7 @@ public class WatchManager extends AbstractLifeCycle { private final ConcurrentMap<Source, ConfigurationMonitor> watchers = new ConcurrentHashMap<>(); public WatchManager(final ConfigurationScheduler scheduler) { - this.scheduler = scheduler; + this.scheduler = Objects.requireNonNull(scheduler, "scheduler"); eventServiceList = getEventServices(); } @@ -184,6 +185,10 @@ public class WatchManager extends AbstractLifeCycle { return this.intervalSeconds; } + ConfigurationScheduler getScheduler() { + return scheduler; + } + /** * Returns a Map of the file watchers. * @@ -306,6 +311,9 @@ public class WatchManager extends AbstractLifeCycle { for (WatchEventService service : eventServiceList) { service.unsubscribe(this); } + if (scheduler.isStarted()) { + scheduler.stop(timeout, timeUnit); + } final boolean stopped = stop(future); setStopped(); return stopped; diff --git a/log4j-core/src/test/java/org/apache/logging/log4j/core/util/WatchHttpTest.java b/log4j-core/src/test/java/org/apache/logging/log4j/core/util/WatchHttpTest.java index 87b1aac..42ce7b1 100644 --- a/log4j-core/src/test/java/org/apache/logging/log4j/core/util/WatchHttpTest.java +++ b/log4j-core/src/test/java/org/apache/logging/log4j/core/util/WatchHttpTest.java @@ -53,6 +53,7 @@ import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo; import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; /** * Test the WatchManager @@ -115,7 +116,7 @@ public class WatchHttpTest { } finally { removeStub(stubMapping); watchManager.stop(); - scheduler.stop(); + assertTrue(watchManager.getScheduler().isStopped()); } } @@ -151,7 +152,7 @@ public class WatchHttpTest { } finally { removeStub(stubMapping); watchManager.stop(); - scheduler.stop(); + assertTrue(watchManager.getScheduler().isStopped()); } } diff --git a/log4j-core/src/test/java/org/apache/logging/log4j/core/util/WatchManagerTest.java b/log4j-core/src/test/java/org/apache/logging/log4j/core/util/WatchManagerTest.java index fecb2e7..c1bd674 100644 --- a/log4j-core/src/test/java/org/apache/logging/log4j/core/util/WatchManagerTest.java +++ b/log4j-core/src/test/java/org/apache/logging/log4j/core/util/WatchManagerTest.java @@ -42,9 +42,24 @@ import static org.junit.jupiter.api.Assertions.*; @EnabledIfSystemProperty(named = "WatchManagerTest.forceRun", matches = "true") public class WatchManagerTest { - private final String testFile = "target/testWatchFile"; - private final String originalFile = "target/test-classes/log4j-test1.xml"; + private static class TestWatcher implements FileWatcher { + + private final Queue<File> queue; + + public TestWatcher(final Queue<File> queue) { + this.queue = queue; + } + + @Override + public void fileModified(final File file) { + System.out.println(file.toString() + " was modified"); + queue.add(file); + } + } + private final String newFile = "target/test-classes/log4j-test1.yaml"; + private final String originalFile = "target/test-classes/log4j-test1.xml"; + private final String testFile = "target/testWatchFile"; @Test public void testWatchManager() throws Exception { @@ -72,7 +87,7 @@ public class WatchManagerTest { assertNotNull(f, "File change not detected"); } finally { watchManager.stop(); - scheduler.stop(); + assertTrue(watchManager.getScheduler().isStopped()); } } @@ -105,7 +120,7 @@ public class WatchManagerTest { assertNull(f, "File change detected"); } finally { watchManager.stop(); - scheduler.stop(); + assertTrue(watchManager.getScheduler().isStopped()); } } @@ -138,22 +153,7 @@ public class WatchManagerTest { assertNull(f, "File change detected"); } finally { watchManager.stop(); - scheduler.stop(); - } - } - - private static class TestWatcher implements FileWatcher { - - private final Queue<File> queue; - - public TestWatcher(final Queue<File> queue) { - this.queue = queue; - } - - @Override - public void fileModified(final File file) { - System.out.println(file.toString() + " was modified"); - queue.add(file); + assertTrue(watchManager.getScheduler().isStopped()); } } } diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 9d2f6f0..a6284d1 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -245,6 +245,9 @@ <action issue="LOG4J2-2916" dev="vy" type="fix" due-to="wuqian0808"> Avoid redundant Kafka producer instantiation causing thread leaks. </action> + <action issue="LOG4J2-3026" dev="ggregory" type="fix" due-to="Gary Gregory"> + WatchManager does not stop its ConfigurationScheduler thereby leaking a thread. + </action> </release> <release version="2.14.0" date="2020-MM-DD" description="GA Release 2.14.0"> <action issue="LOG4J2-2911" dev="rgoers" type="fix">
