Furthermore, you moved everything around in the file which makes it extremely difficult to validate what you actually changed. Please revert this and create a PR that can be reviewed with only the required changes.
Ralph > On Feb 24, 2021, at 8:07 PM, Ralph Goers <[email protected]> wrote: > > Please see my comments on the Jira issue. Without an explanation of how this > accomplishes anything I am -1 on this patch. > > Ralph > >> On Feb 24, 2021, at 2:11 PM, [email protected] wrote: >> >> This is an automated email from the ASF dual-hosted git repository. >> >> ggregory pushed a commit to branch release-2.x >> in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git >> >> commit 86ccf41d6dc999c39ddeb5af1dc0968c167c4643 >> Author: Gary Gregory <[email protected]> >> AuthorDate: Wed Feb 24 16:11:28 2021 -0500 >> >> [LOG4J2-3026] WatchManager does not stop its ConfigurationScheduler >> thereby leaking a thread. >> --- >> .../java/org/apache/logging/log4j/core/util/WatchManager.java | 10 +++++++++- >> .../org/apache/logging/log4j/core/util/WatchHttpTest.java | 5 +++-- >> .../org/apache/logging/log4j/core/util/WatchManagerTest.java | 7 ++++--- >> src/changes/changes.xml | 11 >> +++++++---- >> 4 files changed, 23 insertions(+), 10 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 e760809..48de57d 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 >> @@ -22,6 +22,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 UUID id = LocalUUID.get(); >> >> public WatchManager(final ConfigurationScheduler scheduler) { >> - this.scheduler = scheduler; >> + this.scheduler = Objects.requireNonNull(scheduler, "scheduler"); >> eventServiceList = getEventServices(); >> } >> >> @@ -306,6 +307,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; >> @@ -372,4 +376,8 @@ public class WatchManager extends AbstractLifeCycle { >> Source source = new Source(file); >> watch(source, watcher); >> } >> + >> + ConfigurationScheduler getScheduler() { >> + return scheduler; >> + } >> } >> 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 01b837c..c16d525 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 >> @@ -51,6 +51,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.Assert.assertTrue; >> >> /** >> * Test the WatchManager >> @@ -113,7 +114,7 @@ public class WatchHttpTest { >> } finally { >> removeStub(stubMapping); >> watchManager.stop(); >> - scheduler.stop(); >> + assertTrue(watchManager.getScheduler().isStopped()); >> } >> } >> >> @@ -149,7 +150,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..5902414 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 >> @@ -33,6 +33,7 @@ import org.junit.jupiter.api.condition.DisabledOnOs; >> import org.junit.jupiter.api.condition.EnabledIfSystemProperty; >> import org.junit.jupiter.api.condition.OS; >> >> +import static org.junit.Assert.assertTrue; >> import static org.junit.jupiter.api.Assertions.*; >> >> /** >> @@ -72,7 +73,7 @@ public class WatchManagerTest { >> assertNotNull(f, "File change not detected"); >> } finally { >> watchManager.stop(); >> - scheduler.stop(); >> + assertTrue(watchManager.getScheduler().isStopped()); >> } >> } >> >> @@ -105,7 +106,7 @@ public class WatchManagerTest { >> assertNull(f, "File change detected"); >> } finally { >> watchManager.stop(); >> - scheduler.stop(); >> + assertTrue(watchManager.getScheduler().isStopped()); >> } >> } >> >> @@ -138,7 +139,7 @@ public class WatchManagerTest { >> assertNull(f, "File change detected"); >> } finally { >> watchManager.stop(); >> - scheduler.stop(); >> + assertTrue(watchManager.getScheduler().isStopped()); >> } >> } >> >> diff --git a/src/changes/changes.xml b/src/changes/changes.xml >> index dc548d2..8c6f21f 100644 >> --- a/src/changes/changes.xml >> +++ b/src/changes/changes.xml >> @@ -38,13 +38,13 @@ >> Directly create a thread instead of using the common ForkJoin pool >> when initializing ThreadContextDataInjector" >> </action> >> <action issue="LOG4J2-2624" dev="mattsicker" type="fix" due-to="Tim >> Perry"> >> - Allow auto-shutdown of log4j in log4j-web to be turned off and >> provide a >> + Allow auto-shutdown of log4j in log4j-web to be turned off and >> provide a >> ServletContextListener "Log4jShutdownOnContextDestroyedListener" to >> stop log4j. >> Register the listener at the top of web.xml to ensure the shutdown >> happens last. >> </action> >> <action issue="LOG4J2-1606" dev="mattsicker" type="fix" due-to="Tim >> Perry"> >> - Allow auto-shutdown of log4j in log4j-web to be turned off and >> provide a >> - ServletContextListener "Log4jShutdownOnContextDestroyedListener" to >> stop log4j. >> + Allow auto-shutdown of log4j in log4j-web to be turned off and >> provide a >> + ServletContextListener "Log4jShutdownOnContextDestroyedListener" to >> stop log4j. >> Register the listener at the top of web.xml to ensure the shutdown >> happens last. >> </action> >> <action issue="LOG4J2-2998" dev="vy" type="fix"> >> @@ -83,6 +83,9 @@ >> <action issue="LOG4J2-3014" dev="ggregory" type="fix" due-to="Lee >> Breisacher, Gary Gregory"> >> Log4j1ConfigurationConverter on Windows produces "
" at end of >> every line. >> </action> >> + <action issue="LOG4J2-3026" dev="ggregory" type="fix" due-to="Gary >> Gregory"> >> + WatchManager does not stop its ConfigurationScheduler thereby >> leaking a thread. >> + </action> >> <!-- ADDS --> >> <action issue="LOG4J2-2962" dev="vy" type="add"> >> Enrich "map" resolver by unifying its backend with "mdc" resolver. >> @@ -189,7 +192,7 @@ >> </action> >> <action dev="ggregory" type="update"> >> Update Woodstox 5.0.3 -> 6.2.3 to match Jackson 2.12.1. >> - </action> >> + </action> >> <action dev="ggregory" type="update"> >> Update org.apache.activemq:* 5.16.0 -> 5.16.1. >> </action> >> >> > > >
