diff --git a/CHANGELOG.md b/CHANGELOG.md index dabf36266..e37da8ea9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ All notable changes to the LaunchDarkly Java SDK will be documented in this file. This project adheres to [Semantic Versioning](http://semver.org). +## [5.10.6] - 2023-01-06 +### Fixed: +- Fixed unintended error behavior when the SDK is being shut down, if streaming is enabled. The symptom was that 1. the SDK could log a misleading message about a network error (in reality this was just the connection being deliberately closed) and 2. an uncaught exception could be thrown from the worker thread that managed that connection. The uncaught exception would be ignored in a default JVM configuration, but it could have more serious consequences in an application that had configured a default exception handler to be triggered by all uncaught exceptions. + ## [6.0.2] - 2023-01-04 ### Fixed: - Fixed vulnerability [CVE-2022-1471](https://nvd.nist.gov/vuln/detail/CVE-2022-1471) which could allow arbitrary code execution if using `FileDataSource` with a YAML file. (Thanks, [antonmos](https://github.com/launchdarkly/java-server-sdk/pull/289)!) diff --git a/src/main/java/com/launchdarkly/sdk/server/StreamProcessor.java b/src/main/java/com/launchdarkly/sdk/server/StreamProcessor.java index 37c6dc0f5..a54b291e7 100644 --- a/src/main/java/com/launchdarkly/sdk/server/StreamProcessor.java +++ b/src/main/java/com/launchdarkly/sdk/server/StreamProcessor.java @@ -90,6 +90,7 @@ final class StreamProcessor implements DataSource { private final DataStoreStatusProvider.StatusListener statusListener; private volatile EventSource es; private final AtomicBoolean initialized = new AtomicBoolean(false); + private final AtomicBoolean closed = new AtomicBoolean(false); private volatile long esStarted = 0; private volatile boolean lastStoreUpdateFailed = false; private final LDLogger logger; @@ -182,11 +183,25 @@ public Future start() { // EventSource will start the stream connection either way, but if we called start(), it // would swallow any FaultEvents that happened during the initial conection attempt; we // want to know about those. - for (StreamEvent event: es.anyEvents()) { - if (!handleEvent(event, initFuture)) { - // handleEvent returns false if we should fall through and end the thread - break; + try { + for (StreamEvent event: es.anyEvents()) { + if (!handleEvent(event, initFuture)) { + // handleEvent returns false if we should fall through and end the thread + break; + } + } + } catch (Exception e) { + // Any uncaught runtime exception at this point would be coming from es.anyEvents(). + // That's not expected-- all deliberate EventSource exceptions are checked exceptions. + // So we have to assume something is wrong that we can't recover from at this point, + // and just let the thread terminate. That's better than having the thread be killed + // by an uncaught exception. + if (closed.get()) { + return; // ignore any exception that's just a side effect of stopping the EventSource } + logger.error("Stream thread has ended due to unexpected exception: {}", LogValues.exceptionSummary(e)); + // deliberately log stacktrace at error level since this is an unusual circumstance + logger.error(LogValues.exceptionTrace(e)); } }); thread.setName("LaunchDarkly-streaming"); @@ -205,6 +220,9 @@ private void recordStreamInit(boolean failed) { @Override public void close() throws IOException { + if (closed.getAndSet(true)) { + return; // was already closed + } logger.info("Closing LaunchDarkly StreamProcessor"); if (statusListener != null) { dataSourceUpdates.getDataStoreStatusProvider().removeStatusListener(statusListener); @@ -223,6 +241,9 @@ public boolean isInitialized() { // Handles a single StreamEvent and returns true if we should keep the stream alive, // or false if we should shut down permanently. private boolean handleEvent(StreamEvent event, CompletableFuture initFuture) { + if (closed.get()) { + return false; + } logger.debug("Received StreamEvent: {}", event); if (event instanceof MessageEvent) { handleMessage((MessageEvent)event, initFuture); diff --git a/src/test/java/com/launchdarkly/sdk/server/StreamProcessorTest.java b/src/test/java/com/launchdarkly/sdk/server/StreamProcessorTest.java index 81aebb267..96a8f1240 100644 --- a/src/test/java/com/launchdarkly/sdk/server/StreamProcessorTest.java +++ b/src/test/java/com/launchdarkly/sdk/server/StreamProcessorTest.java @@ -1,6 +1,8 @@ package com.launchdarkly.sdk.server; import com.launchdarkly.eventsource.MessageEvent; +import com.launchdarkly.logging.LDLogLevel; +import com.launchdarkly.logging.LogCapture; import com.launchdarkly.sdk.LDValue; import com.launchdarkly.sdk.internal.events.DiagnosticStore; import com.launchdarkly.sdk.server.DataModel.FeatureFlag; @@ -55,6 +57,7 @@ import static com.launchdarkly.sdk.server.TestComponents.dataSourceUpdates; import static com.launchdarkly.sdk.server.TestUtil.requireDataSourceStatus; import static com.launchdarkly.testhelpers.ConcurrentHelpers.assertFutureIsCompleted; +import static com.launchdarkly.testhelpers.ConcurrentHelpers.assertNoMoreValues; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.endsWith; @@ -63,6 +66,7 @@ import static org.hamcrest.Matchers.lessThanOrEqualTo; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; +import static org.hamcrest.Matchers.startsWith; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; @@ -689,6 +693,39 @@ public void testSpecialHttpConfigurations() throws Exception { }); } + @Test + public void closingStreamProcessorDoesNotLogNetworkError() throws Exception { + // This verifies that we're not generating misleading log output or status updates + // due to simply seeing a broken connection when we have already decided to shut down. + BlockingQueue statuses = new LinkedBlockingQueue<>(); + dataSourceUpdates.statusBroadcaster.register(statuses::add); + + try (HttpServer server = HttpServer.start(streamResponse(EMPTY_DATA_EVENT))) { + try (StreamProcessor sp = createStreamProcessor(null, server.getUri())) { + sp.start(); + dataSourceUpdates.awaitInit(); + requireDataSourceStatus(statuses, State.VALID); + + while (logCapture.awaitMessage(10) != null) {} // drain captured logs + + sp.close(); + + requireDataSourceStatus(statuses, State.OFF); // should not see INTERRUPTED + assertNoMoreValues(statuses, 100, TimeUnit.MILLISECONDS); + + assertThat(logCapture.requireMessage(10).getText(), startsWith("Closing LaunchDarkly")); + // There shouldn't be any other log output other than debugging + for (;;) { + LogCapture.Message message = logCapture.awaitMessage(10); + if (message == null) { + break; + } + assertThat(message.getLevel(), equalTo(LDLogLevel.DEBUG)); + } + } + } + } + private void testUnrecoverableHttpError(int statusCode) throws Exception { Handler errorResp = Handlers.status(statusCode);