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> > >
