From 6ec3c67c6b72537acd7074f1f56f4fbb3f3014d4 Mon Sep 17 00:00:00 2001 From: Phil Clay Date: Sat, 9 Jan 2021 11:49:11 -0800 Subject: [PATCH 1/2] Only stop subcomponents that were started directly. Each component now keeps track of which subcomponents it started, and only stops those that it started directly. Allows lifecycle of subcomponents to be managed outside of the components that use them (e.g. for programmatic configuration, and reuse of subcomponents) --- .../logstash/logback/LifeCycleManager.java | 91 +++++++++++++ .../AbstractLogstashTcpSocketAppender.java | 17 ++- .../AbstractLogstashUdpSocketAppender.java | 10 +- .../appender/AsyncDisruptorAppender.java | 10 +- .../DelegatingAsyncDisruptorAppender.java | 16 ++- .../composite/AbstractNestedJsonProvider.java | 12 +- .../composite/CompositeJsonFormatter.java | 10 +- .../GlobalCustomFieldsJsonProvider.java | 3 + .../logback/composite/JsonProviders.java | 28 +++- .../loggingevent/StackTraceJsonProvider.java | 16 ++- .../logback/encoder/CompositeJsonEncoder.java | 30 +++-- .../logback/layout/CompositeJsonLayout.java | 26 +++- .../status/DelegatingStatusListener.java | 10 +- .../logback/LifeCycleManagerTest.java | 127 ++++++++++++++++++ .../LogstashTcpSocketAppenderTest.java | 34 ++--- .../encoder/CompositeJsonEncoderTest.java | 18 ++- .../LevelFilteringStatusListenerTest.java | 3 + 17 files changed, 392 insertions(+), 69 deletions(-) create mode 100644 src/main/java/net/logstash/logback/LifeCycleManager.java create mode 100644 src/test/java/net/logstash/logback/LifeCycleManagerTest.java diff --git a/src/main/java/net/logstash/logback/LifeCycleManager.java b/src/main/java/net/logstash/logback/LifeCycleManager.java new file mode 100644 index 00000000..b647f6f3 --- /dev/null +++ b/src/main/java/net/logstash/logback/LifeCycleManager.java @@ -0,0 +1,91 @@ +/** + * Licensed 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 net.logstash.logback; + +import java.util.Collections; +import java.util.HashSet; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; + +import ch.qos.logback.core.spi.LifeCycle; + +/** + * Manages the lifecycle of subcomponents. + * + *

Specifically:

+ * + * + */ +public class LifeCycleManager { + + private final Set started = Collections.newSetFromMap(new ConcurrentHashMap<>()); + + /** + * Starts the given lifecycle component if and only if it is not already started. + * + * @param lifeCycle the component to start + * @return true if this method execution started the component + */ + public boolean start(LifeCycle lifeCycle) { + if (lifeCycle.isStarted()) { + return false; + } + lifeCycle.start(); + started.add(lifeCycle); + return true; + } + + /** + * Stops the given lifecycle component if and only if it is currently started, + * AND was started by this lifecycle manager via {@link #start(LifeCycle)}. + * + * @param lifeCycle the component to stop + * @return true if this method execution stopped the component + */ + public boolean stop(LifeCycle lifeCycle) { + if (!lifeCycle.isStarted()) { + return false; + } + if (!started.remove(lifeCycle)) { + return false; + } + lifeCycle.stop(); + return true; + } + + /** + * Stops all of the lifecycle components that were started by {@link #start(LifeCycle)} + * and are currently started. + * + * @return the lifecycle components that this method execution stopped + */ + public Set stopAll() { + Set stopped = new HashSet<>(); + + for (LifeCycle lifeCycle : started) { + if (lifeCycle.isStarted()) { + lifeCycle.stop(); + stopped.add(lifeCycle); + } + } + + started.clear(); + return Collections.unmodifiableSet(stopped); + } + + +} diff --git a/src/main/java/net/logstash/logback/appender/AbstractLogstashTcpSocketAppender.java b/src/main/java/net/logstash/logback/appender/AbstractLogstashTcpSocketAppender.java index 3909b972..cf06aaa5 100644 --- a/src/main/java/net/logstash/logback/appender/AbstractLogstashTcpSocketAppender.java +++ b/src/main/java/net/logstash/logback/appender/AbstractLogstashTcpSocketAppender.java @@ -42,6 +42,7 @@ import javax.net.ssl.SSLSocket; import javax.net.ssl.SSLSocketFactory; +import net.logstash.logback.LifeCycleManager; import net.logstash.logback.appender.destination.DelegateDestinationConnectionStrategy; import net.logstash.logback.appender.destination.DestinationConnectionStrategy; import net.logstash.logback.appender.destination.DestinationParser; @@ -359,6 +360,11 @@ private class TcpSendingEventHandler implements EventHandler>, L */ private Future readerFuture; + /** + * Manages the lifecycle of subcomponents + */ + private final LifeCycleManager lifecycleManager = new LifeCycleManager(); + /** * When run, if the {@link AbstractLogstashTcpSocketAppender#keepAliveDuration} * has elapsed since the last event was sent, @@ -631,6 +637,7 @@ private boolean hasKeepAliveDurationElapsed(long lastSentNanoTime, long currentN public void onStart() { this.destinationAttemptStartTimes = new long[destinations.size()]; openSocket(); + lifecycleManager.start(encoder); scheduleKeepAlive(System.nanoTime()); scheduleWriteTimeout(); } @@ -639,7 +646,7 @@ public void onStart() { public void onShutdown() { unscheduleWriteTimeout(); unscheduleKeepAlive(); - closeEncoder(); + lifecycleManager.stop(encoder); closeSocket(); } @@ -788,10 +795,6 @@ private synchronized void closeSocket() { } } - private void closeEncoder() { - encoder.stop(); - } - private synchronized void scheduleKeepAlive(long basedOnNanoTime) { if (isKeepAliveEnabled() && !Thread.currentThread().isInterrupted()) { if (keepAliveRunnable == null) { @@ -967,10 +970,6 @@ public synchronized void start() { if (errorCount == 0) { encoder.setContext(getContext()); - if (!encoder.isStarted()) { - encoder.start(); - } - /* * Increase the core size to handle the reader thread */ diff --git a/src/main/java/net/logstash/logback/appender/AbstractLogstashUdpSocketAppender.java b/src/main/java/net/logstash/logback/appender/AbstractLogstashUdpSocketAppender.java index 1c690f74..38207fc3 100644 --- a/src/main/java/net/logstash/logback/appender/AbstractLogstashUdpSocketAppender.java +++ b/src/main/java/net/logstash/logback/appender/AbstractLogstashUdpSocketAppender.java @@ -24,6 +24,7 @@ import ch.qos.logback.core.net.SyslogAppenderBase; import ch.qos.logback.core.net.SyslogOutputStream; import ch.qos.logback.core.spi.DeferredProcessingAware; +import net.logstash.logback.LifeCycleManager; import net.logstash.logback.appender.listener.AppenderListener; /** @@ -40,6 +41,11 @@ public class AbstractLogstashUdpSocketAppender listeners = new ArrayList<>(); + /** + * Manages the lifecycle of subcomponents + */ + private final LifeCycleManager lifecycleManager = new LifeCycleManager(); + /** * Event wrapper object used for each element of the {@link RingBuffer}. */ @@ -369,7 +375,7 @@ public void start() { statusListener.setLevelValue(Status.WARN); statusListener.setDelegate(new OnConsoleStatusListener()); statusListener.setContext(getContext()); - statusListener.start(); + lifecycleManager.start(statusListener); getStatusManager().add(statusListener); } @@ -439,6 +445,8 @@ public void stop() { } catch (InterruptedException e) { addWarn("Some queued events have not been logged due to requested shutdown", e); } + + lifecycleManager.stopAll(); fireAppenderStopped(); } diff --git a/src/main/java/net/logstash/logback/appender/DelegatingAsyncDisruptorAppender.java b/src/main/java/net/logstash/logback/appender/DelegatingAsyncDisruptorAppender.java index 352687c9..5e39cc28 100644 --- a/src/main/java/net/logstash/logback/appender/DelegatingAsyncDisruptorAppender.java +++ b/src/main/java/net/logstash/logback/appender/DelegatingAsyncDisruptorAppender.java @@ -30,6 +30,7 @@ import ch.qos.logback.core.spi.AppenderAttachable; import ch.qos.logback.core.spi.AppenderAttachableImpl; import ch.qos.logback.core.spi.DeferredProcessingAware; +import net.logstash.logback.LifeCycleManager; import net.logstash.logback.appender.listener.AppenderListener; /** @@ -52,7 +53,12 @@ public abstract class DelegatingAsyncDisruptorAppender appenders = new AppenderAttachableImpl(); - + + /** + * Manages the lifecycle of subcomponents + */ + private final LifeCycleManager lifecycleManager = new LifeCycleManager(); + private class DelegatingEventHandler implements EventHandler> { /** * Whether exceptions should be reported with a error status or not. @@ -142,18 +148,14 @@ private void startDelegateAppenders() { if (appender.getContext() == null) { appender.setContext(getContext()); } - if (!appender.isStarted()) { - appender.start(); - } + lifecycleManager.start(appender); } } private void stopDelegateAppenders() { for (Iterator> appenderIter = appenders.iteratorForAppenders(); appenderIter.hasNext();) { Appender appender = appenderIter.next(); - if (appender.isStarted()) { - appender.stop(); - } + lifecycleManager.stop(appender); } } diff --git a/src/main/java/net/logstash/logback/composite/AbstractNestedJsonProvider.java b/src/main/java/net/logstash/logback/composite/AbstractNestedJsonProvider.java index 34f615a7..bfc84516 100644 --- a/src/main/java/net/logstash/logback/composite/AbstractNestedJsonProvider.java +++ b/src/main/java/net/logstash/logback/composite/AbstractNestedJsonProvider.java @@ -21,6 +21,7 @@ import ch.qos.logback.access.spi.IAccessEvent; import ch.qos.logback.classic.spi.ILoggingEvent; import ch.qos.logback.core.spi.DeferredProcessingAware; +import net.logstash.logback.LifeCycleManager; /** * A {@link JsonProvider} that nests other providers within a subobject. @@ -35,7 +36,12 @@ public abstract class AbstractNestedJsonProvider jsonProviders = new JsonProviders(); - + + /** + * Manages the lifecycle of subcomponents + */ + private final LifeCycleManager lifecycleManager = new LifeCycleManager(); + public AbstractNestedJsonProvider() { setFieldName(FIELD_NESTED); } @@ -43,13 +49,13 @@ public AbstractNestedJsonProvider() { @Override public void start() { super.start(); - getProviders().start(); + lifecycleManager.start(getProviders()); } @Override public void stop() { super.stop(); - getProviders().stop(); + lifecycleManager.stop(getProviders()); } @Override diff --git a/src/main/java/net/logstash/logback/composite/CompositeJsonFormatter.java b/src/main/java/net/logstash/logback/composite/CompositeJsonFormatter.java index 4ac1ee03..7b9ff1c1 100644 --- a/src/main/java/net/logstash/logback/composite/CompositeJsonFormatter.java +++ b/src/main/java/net/logstash/logback/composite/CompositeJsonFormatter.java @@ -19,6 +19,7 @@ import java.lang.ref.SoftReference; import java.util.ServiceConfigurationError; +import net.logstash.logback.LifeCycleManager; import net.logstash.logback.decorate.JsonFactoryDecorator; import net.logstash.logback.decorate.JsonGeneratorDecorator; import net.logstash.logback.decorate.NullJsonFactoryDecorator; @@ -86,6 +87,11 @@ protected SoftReference initialValue() { */ private JsonProviders jsonProviders = new JsonProviders(); + /** + * Manages the lifecycle of subcomponents + */ + private final LifeCycleManager lifecycleManager = new LifeCycleManager(); + private JsonEncoding encoding = JsonEncoding.UTF8; private boolean findAndRegisterJacksonModules = true; @@ -104,13 +110,13 @@ public void start() { jsonFactory = createJsonFactory(); jsonProviders.setContext(context); jsonProviders.setJsonFactory(jsonFactory); - jsonProviders.start(); + lifecycleManager.start(jsonProviders); started = true; } @Override public void stop() { - jsonProviders.stop(); + lifecycleManager.stop(jsonProviders); started = false; } diff --git a/src/main/java/net/logstash/logback/composite/GlobalCustomFieldsJsonProvider.java b/src/main/java/net/logstash/logback/composite/GlobalCustomFieldsJsonProvider.java index 2f37cbfc..cb7154ee 100644 --- a/src/main/java/net/logstash/logback/composite/GlobalCustomFieldsJsonProvider.java +++ b/src/main/java/net/logstash/logback/composite/GlobalCustomFieldsJsonProvider.java @@ -98,5 +98,8 @@ public void setCustomFieldsNode(JsonNode customFields) { @Override public void setJsonFactory(JsonFactory jsonFactory) { this.jsonFactory = jsonFactory; + if (isStarted()) { + initializeCustomFields(); + } } } diff --git a/src/main/java/net/logstash/logback/composite/JsonProviders.java b/src/main/java/net/logstash/logback/composite/JsonProviders.java index 0eecf9ba..a471ef95 100644 --- a/src/main/java/net/logstash/logback/composite/JsonProviders.java +++ b/src/main/java/net/logstash/logback/composite/JsonProviders.java @@ -22,6 +22,8 @@ import ch.qos.logback.classic.spi.ILoggingEvent; import ch.qos.logback.core.Context; import ch.qos.logback.core.spi.DeferredProcessingAware; +import ch.qos.logback.core.spi.LifeCycle; +import net.logstash.logback.LifeCycleManager; import com.fasterxml.jackson.core.JsonFactory; import com.fasterxml.jackson.core.JsonGenerator; @@ -38,20 +40,36 @@ * * @param type of event ({@link ILoggingEvent} or {@link IAccessEvent}). */ -public class JsonProviders implements JsonFactoryAware { +public class JsonProviders implements JsonFactoryAware, LifeCycle { private final List> jsonProviders = new ArrayList>(); + private boolean started; + + /** + * Manages the lifecycle of subcomponents + */ + private final LifeCycleManager lifecycleManager = new LifeCycleManager(); + public void start() { - for (JsonProvider jsonProvider : jsonProviders) { - jsonProvider.start(); + if (isStarted()) { + return; } + jsonProviders.forEach(lifecycleManager::start); + started = true; } public void stop() { - for (JsonProvider jsonProvider : jsonProviders) { - jsonProvider.stop(); + if (!isStarted()) { + return; } + jsonProviders.forEach(lifecycleManager::stop); + started = false; + } + + @Override + public boolean isStarted() { + return started; } public void setContext(Context context) { diff --git a/src/main/java/net/logstash/logback/composite/loggingevent/StackTraceJsonProvider.java b/src/main/java/net/logstash/logback/composite/loggingevent/StackTraceJsonProvider.java index e4fe19be..1b77f79f 100644 --- a/src/main/java/net/logstash/logback/composite/loggingevent/StackTraceJsonProvider.java +++ b/src/main/java/net/logstash/logback/composite/loggingevent/StackTraceJsonProvider.java @@ -21,6 +21,7 @@ import ch.qos.logback.classic.pattern.ThrowableHandlingConverter; import ch.qos.logback.classic.spi.ILoggingEvent; import ch.qos.logback.classic.spi.IThrowableProxy; +import net.logstash.logback.LifeCycleManager; import net.logstash.logback.composite.AbstractFieldJsonProvider; import net.logstash.logback.composite.FieldNamesAware; import net.logstash.logback.composite.JsonWritingUtils; @@ -41,19 +42,30 @@ public class StackTraceJsonProvider extends AbstractFieldJsonProvider wrapped, Event event) { public void start() { super.start(); formatter.setContext(getContext()); - formatter.start(); + lifecycleManager.start(formatter); charset = Charset.forName(formatter.getEncoding()); lineSeparatorBytes = this.lineSeparator == null ? EMPTY_BYTES @@ -121,6 +127,9 @@ public void start() { @SuppressWarnings({ "unchecked", "rawtypes" }) private void startWrapped(Encoder wrapped) { + if (wrapped == null) { + return; + } if (wrapped instanceof LayoutWrappingEncoder) { /* * Convenience hack to ensure the same charset is used in most cases. @@ -146,24 +155,29 @@ private void startWrapped(Encoder wrapped) { layout.start(); } } - - if (wrapped != null && !wrapped.isStarted()) { - wrapped.start(); - } + lifecycleManager.start(wrapped); } @Override public void stop() { super.stop(); - formatter.stop(); + lifecycleManager.stop(formatter); stopWrapped(prefix); stopWrapped(suffix); } private void stopWrapped(Encoder wrapped) { - if (wrapped != null && !wrapped.isStarted()) { - wrapped.stop(); + if (wrapped == null) { + return; + } + if (wrapped instanceof LayoutWrappingEncoder) { + LayoutWrappingEncoder layoutWrappedEncoder = (LayoutWrappingEncoder) wrapped; + if (layoutWrappedEncoder.getLayout() instanceof PatternLayoutBase) { + PatternLayoutBase layout = (PatternLayoutBase) layoutWrappedEncoder.getLayout(); + lifecycleManager.stop(layout); + } } + lifecycleManager.stop(wrapped); } @Override diff --git a/src/main/java/net/logstash/logback/layout/CompositeJsonLayout.java b/src/main/java/net/logstash/logback/layout/CompositeJsonLayout.java index 0c8b15e7..1dbc855e 100644 --- a/src/main/java/net/logstash/logback/layout/CompositeJsonLayout.java +++ b/src/main/java/net/logstash/logback/layout/CompositeJsonLayout.java @@ -15,6 +15,7 @@ import java.io.IOException; +import net.logstash.logback.LifeCycleManager; import net.logstash.logback.composite.CompositeJsonFormatter; import net.logstash.logback.composite.JsonProviders; import net.logstash.logback.decorate.JsonFactoryDecorator; @@ -45,6 +46,11 @@ public abstract class CompositeJsonLayout private final CompositeJsonFormatter formatter; + /** + * Manages the lifecycle of subcomponents + */ + private final LifeCycleManager lifecycleManager = new LifeCycleManager(); + public CompositeJsonLayout() { super(); this.formatter = createFormatter(); @@ -98,12 +104,15 @@ private String doLayoutWrapped(Layout wrapped, Event event) { public void start() { super.start(); formatter.setContext(getContext()); - formatter.start(); + lifecycleManager.start(formatter); startWrapped(prefix); startWrapped(suffix); } private void startWrapped(Layout wrapped) { + if (wrapped == null) { + return; + } if (wrapped instanceof PatternLayoutBase) { /* * Don't ensure exception output (for ILoggingEvents) @@ -118,23 +127,26 @@ private void startWrapped(Layout wrapped) { */ layout.start(); } - if (wrapped != null && !wrapped.isStarted()) { - wrapped.start(); - } + lifecycleManager.start(wrapped); } @Override public void stop() { super.stop(); - formatter.stop(); + lifecycleManager.stop(formatter); stopWrapped(prefix); stopWrapped(suffix); } private void stopWrapped(Layout wrapped) { - if (wrapped != null && !wrapped.isStarted()) { - wrapped.stop(); + if (wrapped == null) { + return; + } + if (wrapped instanceof PatternLayoutBase) { + PatternLayoutBase layout = (PatternLayoutBase) wrapped; + layout.stop(); } + lifecycleManager.stop(wrapped); } public JsonProviders getProviders() { diff --git a/src/main/java/net/logstash/logback/status/DelegatingStatusListener.java b/src/main/java/net/logstash/logback/status/DelegatingStatusListener.java index a243fa4d..f2d6a294 100644 --- a/src/main/java/net/logstash/logback/status/DelegatingStatusListener.java +++ b/src/main/java/net/logstash/logback/status/DelegatingStatusListener.java @@ -19,6 +19,7 @@ import ch.qos.logback.core.spi.LifeCycle; import ch.qos.logback.core.status.Status; import ch.qos.logback.core.status.StatusListener; +import net.logstash.logback.LifeCycleManager; /** * A {@link StatusListener} that delegates to another {@link StatusListener} @@ -29,6 +30,11 @@ public class DelegatingStatusListener extends ContextAwareBase implements Status private volatile boolean started; + /** + * Manages the lifecycle of subcomponents + */ + private final LifeCycleManager lifecycleManager = new LifeCycleManager(); + @Override public void start() { if (delegate == null) { @@ -39,7 +45,7 @@ public void start() { ((ContextAware) delegate).setContext(context); } if (delegate instanceof LifeCycle) { - ((LifeCycle) delegate).start(); + lifecycleManager.start((LifeCycle) delegate); } started = true; } @@ -47,7 +53,7 @@ public void start() { @Override public void stop() { if (delegate instanceof LifeCycle) { - ((LifeCycle) delegate).stop(); + lifecycleManager.stop((LifeCycle) delegate); } started = false; } diff --git a/src/test/java/net/logstash/logback/LifeCycleManagerTest.java b/src/test/java/net/logstash/logback/LifeCycleManagerTest.java new file mode 100644 index 00000000..bb3ab1c1 --- /dev/null +++ b/src/test/java/net/logstash/logback/LifeCycleManagerTest.java @@ -0,0 +1,127 @@ +/** + * Licensed 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 net.logstash.logback; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.Set; + +import ch.qos.logback.core.spi.LifeCycle; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) +class LifeCycleManagerTest { + + @Mock + private LifeCycle lifeCycle; + + @Test + void original_not_started() { + + LifeCycleManager lifecycleManager = new LifeCycleManager(); + lifecycleManager.start(lifeCycle); + + verify(lifeCycle).start(); + + when(lifeCycle.isStarted()).thenReturn(true); + + lifecycleManager.stop(lifeCycle); + + verify(lifeCycle).stop(); + + } + + @Test + void original_started() { + when(lifeCycle.isStarted()).thenReturn(true); + + LifeCycleManager lifecycleManager = new LifeCycleManager(); + lifecycleManager.start(lifeCycle); + + verify(lifeCycle, never()).start(); + + lifecycleManager.stop(lifeCycle); + + verify(lifeCycle, never()).stop(); + + } + + @Test + void stopped_manually() { + + LifeCycleManager lifecycleManager = new LifeCycleManager(); + lifecycleManager.start(lifeCycle); + + verify(lifeCycle).start(); + + when(lifeCycle.isStarted()).thenReturn(false); + + lifecycleManager.stop(lifeCycle); + + verify(lifeCycle, never()).stop(); + + } + + @Test + void unknown_stopped() { + + LifeCycleManager lifecycleManager = new LifeCycleManager(); + + when(lifeCycle.isStarted()).thenReturn(true); + + lifecycleManager.stop(lifeCycle); + + verify(lifeCycle, never()).stop(); + + } + + @Test + void stopAll() { + + LifeCycle lifeCycle1 = mock(LifeCycle.class); + LifeCycle lifeCycle2 = mock(LifeCycle.class); + LifeCycle lifeCycle3 = mock(LifeCycle.class); + + when(lifeCycle2.isStarted()).thenReturn(true); + + LifeCycleManager lifecycleManager = new LifeCycleManager(); + lifecycleManager.start(lifeCycle1); + lifecycleManager.start(lifeCycle2); + lifecycleManager.start(lifeCycle3); + + verify(lifeCycle1).start(); + verify(lifeCycle2, never()).start(); + verify(lifeCycle3).start(); + + when(lifeCycle1.isStarted()).thenReturn(true); + + Set stopped = lifecycleManager.stopAll(); + + assertThat(stopped).containsExactly(lifeCycle1); + + verify(lifeCycle1).stop(); + verify(lifeCycle2, never()).stop(); + verify(lifeCycle3, never()).stop(); + + } + + +} \ No newline at end of file diff --git a/src/test/java/net/logstash/logback/appender/LogstashTcpSocketAppenderTest.java b/src/test/java/net/logstash/logback/appender/LogstashTcpSocketAppenderTest.java index 7bab177b..28a424c0 100644 --- a/src/test/java/net/logstash/logback/appender/LogstashTcpSocketAppenderTest.java +++ b/src/test/java/net/logstash/logback/appender/LogstashTcpSocketAppenderTest.java @@ -133,7 +133,7 @@ public void testEncoderCalled_logback12OrLater() { appender.start(); - verify(encoder).start(); + verify(encoder, timeout(VERIFICATION_TIMEOUT)).start();; appender.append(event1); @@ -155,7 +155,7 @@ public void testReconnectOnOpen() throws Exception { appender.start(); - verify(encoder).start(); + verify(encoder, timeout(VERIFICATION_TIMEOUT)).start();; appender.append(event1); @@ -182,8 +182,8 @@ public void testReconnectOnWrite() { appender.setReconnectionDelay(new Duration(100)); appender.start(); - - verify(encoder).start(); + + verify(encoder, timeout(VERIFICATION_TIMEOUT)).start(); doThrow(new RuntimeException()).doReturn("event1".getBytes(StandardCharsets.UTF_8)).when(encoder).encode(event1); @@ -210,7 +210,7 @@ public void testReconnectOnReadFailure() { appender.start(); - verify(encoder).start(); + verify(encoder, timeout(VERIFICATION_TIMEOUT)).start();; appender.append(event1); @@ -230,7 +230,7 @@ public void testConnectOnPrimary() throws Exception { appender.addDestination("localhost:10001"); appender.start(); - verify(encoder).start(); + verify(encoder, timeout(VERIFICATION_TIMEOUT)).start();; // Only one socket should have been created verify(socket, timeout(VERIFICATION_TIMEOUT).times(1)).connect(any(SocketAddress.class), anyInt()); @@ -261,7 +261,7 @@ public void testReconnectToSecondaryOnOpen() throws Exception { // Start the appender and verify it is actually started. // It should try to connect to primary, fail then retry on secondary. appender.start(); - verify(encoder).start(); + verify(encoder, timeout(VERIFICATION_TIMEOUT)).start();; // TWO connection attempts must have been made (without delay) verify(socket, timeout(VERIFICATION_TIMEOUT).times(2)).connect(any(SocketAddress.class), anyInt()); @@ -292,7 +292,7 @@ public void testRandomDestinationAndReconnectToSecondaryOnOpen() throws Exceptio // Start the appender and verify it is actually started. // It should try to connect to second destination by random destination, fail then retry on first destination. appender.start(); - verify(encoder).start(); + verify(encoder, timeout(VERIFICATION_TIMEOUT)).start(); // TWO connection attempts must have been made (without delay) verify(socket, timeout(VERIFICATION_TIMEOUT).times(2)).connect(any(SocketAddress.class), anyInt()); @@ -337,7 +337,7 @@ public void testReconnectToSecondaryOnWrite() throws Exception { // Start the appender and verify it is actually started // At this point, it should be connected to primary. appender.start(); - verify(encoder).start(); + verify(encoder, timeout(VERIFICATION_TIMEOUT)).start();; appender.append(event1); @@ -373,7 +373,7 @@ public void testReconnectToPrimaryWhileOnSecondary() throws Exception { // Start the appender and verify it is actually started // At this point, it should be connected to primary. appender.start(); - verify(encoder).start(); + verify(encoder, timeout(VERIFICATION_TIMEOUT)).start();; // The appender is supposed to be on the secondary. @@ -424,7 +424,7 @@ public void testReconnectWaitWhenExhausted() throws Exception { // Start the appender and verify it is actually started // At this point, it should be connected to primary. appender.start(); - verify(encoder).start(); + verify(encoder, timeout(VERIFICATION_TIMEOUT)).start();; // THREE connection attempts must have been made in total @@ -468,7 +468,7 @@ public void testKeepAlive() throws Exception { // Start the appender and verify it is actually started // At this point, it should be connected to primary. appender.start(); - verify(encoder).start(); + verify(encoder, timeout(VERIFICATION_TIMEOUT)).start();; // Wait for a bit more than 2 keep alive messages then make sure we got the expected content Thread.sleep(250); @@ -510,12 +510,12 @@ public void testWriteTimeout() throws Exception { // Start the appender and verify it is actually started // At this point, it should be connected to primary. appender.start(); - verify(encoder).start(); + verify(encoder, timeout(VERIFICATION_TIMEOUT)).start(); appender.append(event1); - verify(goodOutputStream, timeout(1000)).write(any()); - verify(goodOutputStream, timeout(1000)).flush(); + verify(goodOutputStream, timeout(VERIFICATION_TIMEOUT)).write(any()); + verify(goodOutputStream, timeout(VERIFICATION_TIMEOUT)).flush(); } /** @@ -550,7 +550,7 @@ public void testReconnectToSecondaryOnKeepAlive() throws Exception { // Start the appender and verify it is actually started // At this point, it should be connected to primary. appender.start(); - verify(encoder).start(); + verify(encoder, timeout(VERIFICATION_TIMEOUT)).start();; // Wait for a bit more than a single keep alive message. // TWO connection attempts must have been made in total: @@ -601,7 +601,7 @@ public void testRoundRobin() throws Exception { appender.start(); - verify(encoder).start(); + verify(encoder, timeout(VERIFICATION_TIMEOUT)).start();; appender.append(event1); diff --git a/src/test/java/net/logstash/logback/encoder/CompositeJsonEncoderTest.java b/src/test/java/net/logstash/logback/encoder/CompositeJsonEncoderTest.java index 857fcac5..b822bef8 100644 --- a/src/test/java/net/logstash/logback/encoder/CompositeJsonEncoderTest.java +++ b/src/test/java/net/logstash/logback/encoder/CompositeJsonEncoderTest.java @@ -74,7 +74,7 @@ public void setup() { } @Test - public void testNoPrefixNoSuffix_logback12OrLater() throws IOException { + public void testNoPrefixNoSuffix() throws IOException { encoder.start(); @@ -88,14 +88,16 @@ public void testNoPrefixNoSuffix_logback12OrLater() throws IOException { verify(formatter).writeEventToOutputStream(eq(event), any(OutputStream.class)); assertThat(encoded).containsExactly(System.getProperty("line.separator").getBytes(StandardCharsets.UTF_8)); - + + when(formatter.isStarted()).thenReturn(true); + encoder.stop(); Assertions.assertFalse(encoder.isStarted()); verify(formatter).stop(); } @Test - public void testPrefixAndSuffix_logback12OrLater() throws IOException { + public void testPrefixAndSuffix() throws IOException { LayoutWrappingEncoder prefix = mock(LayoutWrappingEncoder.class); Encoder suffix = mock(Encoder.class); @@ -125,7 +127,15 @@ public void testPrefixAndSuffix_logback12OrLater() throws IOException { verify(formatter).writeEventToOutputStream(eq(event), any(OutputStream.class)); assertThat(encoded).containsExactly(("prefixsuffix" + System.getProperty("line.separator")).getBytes(StandardCharsets.UTF_8)); - + + when(formatter.isStarted()).thenReturn(true); + when(prefix.isStarted()).thenReturn(true); + when(suffix.isStarted()).thenReturn(true); + + when(formatter.isStarted()).thenReturn(true); + when(prefix.isStarted()).thenReturn(true); + when(suffix.isStarted()).thenReturn(true); + encoder.stop(); Assertions.assertFalse(encoder.isStarted()); verify(formatter).stop(); diff --git a/src/test/java/net/logstash/logback/status/LevelFilteringStatusListenerTest.java b/src/test/java/net/logstash/logback/status/LevelFilteringStatusListenerTest.java index 3d10429e..0a1bfd78 100644 --- a/src/test/java/net/logstash/logback/status/LevelFilteringStatusListenerTest.java +++ b/src/test/java/net/logstash/logback/status/LevelFilteringStatusListenerTest.java @@ -17,6 +17,7 @@ import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; import ch.qos.logback.core.Context; import ch.qos.logback.core.status.ErrorStatus; @@ -93,6 +94,8 @@ public void onConsoleStatusListener_warn() { statusListener.addStatusEvent(ERROR_STATUS); verify(onConsoleStatusListener).addStatusEvent(ERROR_STATUS); + when(onConsoleStatusListener.isStarted()).thenReturn(true); + statusListener.stop(); verify(onConsoleStatusListener).stop(); assertThat(statusListener.isStarted()).isFalse(); From 45113f8123fcd8307eefbc9921c77dcd0a253a5f Mon Sep 17 00:00:00 2001 From: Phil Clay Date: Sat, 9 Jan 2021 15:20:50 -0800 Subject: [PATCH 2/2] Make LifeCycleManager.stopAll more threadsafe --- src/main/java/net/logstash/logback/LifeCycleManager.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/main/java/net/logstash/logback/LifeCycleManager.java b/src/main/java/net/logstash/logback/LifeCycleManager.java index b647f6f3..5bf2dafe 100644 --- a/src/main/java/net/logstash/logback/LifeCycleManager.java +++ b/src/main/java/net/logstash/logback/LifeCycleManager.java @@ -15,6 +15,7 @@ import java.util.Collections; import java.util.HashSet; +import java.util.Iterator; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; @@ -76,14 +77,14 @@ public boolean stop(LifeCycle lifeCycle) { public Set stopAll() { Set stopped = new HashSet<>(); - for (LifeCycle lifeCycle : started) { + for (Iterator iterator = started.iterator(); iterator.hasNext();) { + LifeCycle lifeCycle = iterator.next(); if (lifeCycle.isStarted()) { lifeCycle.stop(); stopped.add(lifeCycle); } + iterator.remove(); } - - started.clear(); return Collections.unmodifiableSet(stopped); }