You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@logging.apache.org by Ralph Goers <ra...@dslextreme.com> on 2021/02/25 03:07:34 UTC
Re: [logging-log4j2] 02/02: [LOG4J2-3026] WatchManager does not stop
its ConfigurationScheduler thereby leaking a thread.
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, ggregory@apache.org 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 <ga...@gmail.com>
> 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>
>
>
Re: [logging-log4j2] 02/02: [LOG4J2-3026] WatchManager does not stop
its ConfigurationScheduler thereby leaking a thread.
Posted by Ralph Goers <ra...@dslextreme.com>.
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 <ra...@dslextreme.com> 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, ggregory@apache.org 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 <ga...@gmail.com>
>> 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>
>>
>>
>
>
>