Skip to content

Commit 770f0a9

Browse files
authored
fix: Open a new connection on each stream reconnect (#232)
## Summary After a stream read timeout on an HTTP/2 connection, OkHttp keeps the connection in the pool and treats it as healthy until a 1 second ping deadline passes. After a long-lived stream fails, the first reconnect comes 0.5 to 1 second later. So when the network silently drops the stream connection, the reconnect opens a new stream on the same dead connection and waits for another 5 minute read timeout. Recovery takes about 10 minutes instead of 5. The stream's OkHttp client now uses a connection pool that keeps no idle connections, so each reconnect opens a new connection. This applies to the FDv1 streaming data source and the FDv2 streaming synchronizer. The stream client has its own pool, so polling and event delivery are unchanged. This also updates okhttp-eventsource to 5.0.1, which closes the response body when a stream stops (launchdarkly/okhttp-eventsource#114). Without that, a restart that the SDK starts itself, such as after an FDv2 `goodbye` or invalid data, leaves the abandoned HTTP/2 call holding the old connection, so this pool setting cannot close it. New tests count TCP connections across a reconnect and fail without this change. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Fixes slow recovery when an SSE stream dies on HTTP/2 by forcing **each stream reconnect to use a fresh TCP connection** instead of reusing a pooled connection OkHttp may still treat as healthy for ~1s after a read timeout. > > **StreamProcessor** (FDv1) and **StreamingSynchronizerImpl** (FDv2) now configure the eventsource OkHttp client with `ConnectionPool(0, 1, SECONDS)` **after** `httpProperties` applies its default pool, so idle connections are not kept for streaming only. Polling and other HTTP clients are unchanged. > > Bumps **okhttp-eventsource** to `5.0.1` (pairs with upstream fix for abandoned HTTP/2 calls on SDK-initiated restarts). Adds integration tests that count TCP connections across a reconnect and expect two connections. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 332fbe0. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 355bae8 commit 770f0a9

5 files changed

Lines changed: 99 additions & 1 deletion

File tree

‎lib/sdk/server/build.gradle‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ ext.versions = [
7474
"launchdarklyJavaSdkInternal": "1.12.2",
7575
"launchdarklyLogging": "1.1.0",
7676
"okhttp": "4.12.0", // specify this for the SDK build instead of relying on the transitive dependency from okhttp-eventsource
77-
"okhttpEventsource": "5.0.0",
77+
"okhttpEventsource": "5.0.1",
7878
"reactorCore":"3.3.22.RELEASE",
7979
"slf4j": "1.7.36",
8080
"snakeyaml": "2.4",

‎lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/StreamProcessor.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
import java.util.concurrent.atomic.AtomicBoolean;
5050
import java.util.function.Function;
5151

52+
import okhttp3.ConnectionPool;
5253
import okhttp3.Headers;
5354

5455
/**
@@ -191,11 +192,17 @@ public Future<Void> start() {
191192
// smaller one there because we don't expect long delays within any *non*-streaming response that the
192193
// LD client gets. A read timeout on the stream will result in the connection being cycled, so we set
193194
// this to be slightly more than the expected interval between heartbeat signals.
195+
//
196+
// 3. The connection pool keeps no idle connections, so each reconnect opens a new connection. After a
197+
// stream read timeout, OkHttp can treat a dead HTTP/2 connection as healthy for up to 1 second. A
198+
// reconnect in that time uses the dead connection and stalls until the next read timeout.
194199

195200
HttpConnectStrategy eventSourceHttpConfig = ConnectStrategy.http(this.streamUri)
196201
.headers(headers)
197202
.clientBuilderActions(clientBuilder -> {
198203
httpProperties.applyToHttpClientBuilder(clientBuilder);
204+
// Set the connection pool after httpProperties, which sets its own pool (see note 3 above).
205+
clientBuilder.connectionPool(new ConnectionPool(0, 1, TimeUnit.SECONDS));
199206
})
200207
// Set readTimeout last, to ensure that this hard-coded value overrides any other read
201208
// timeout that might have been set by httpProperties (see comment about readTimeout above).

‎lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/StreamingSynchronizerImpl.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import com.launchdarkly.sdk.server.subsystems.DataStoreTypes.KeyedItems;
3232
import com.launchdarkly.sdk.server.subsystems.SerializationException;
3333
import com.google.gson.stream.JsonReader;
34+
import okhttp3.ConnectionPool;
3435
import okhttp3.Headers;
3536
import org.jetbrains.annotations.NotNull;
3637

@@ -109,6 +110,11 @@ private void startStream() {
109110
.headers(headers)
110111
.clientBuilderActions(clientBuilder -> {
111112
httpProperties.applyToHttpClientBuilder(clientBuilder);
113+
// Keep no idle connections, so each reconnect opens a new connection. After a
114+
// stream read timeout, OkHttp can treat a dead HTTP/2 connection as healthy for
115+
// up to 1 second. A reconnect in that time uses the dead connection and stalls
116+
// until the next read timeout. Set this after httpProperties, which sets its own pool.
117+
clientBuilder.connectionPool(new ConnectionPool(0, 1, TimeUnit.SECONDS));
112118
// Add interceptor to inject selector and filter query parameters on each request
113119
clientBuilder.addInterceptor(chain -> {
114120
okhttp3.Request originalRequest = chain.request();

‎lib/sdk/server/src/test/java/com/launchdarkly/sdk/server/StreamProcessorTest.java‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -422,6 +422,36 @@ public void streamWillReconnectAfterGeneralIOException() throws Exception {
422422
}
423423
}
424424

425+
@Test
426+
public void streamReconnectOpensNewConnection() throws Exception {
427+
// The first stream ends normally. This leaves a connection that the HTTP client could reuse.
428+
Semaphore closeFirstStream = new Semaphore(0);
429+
Handler streamHandler = Handlers.sequential(
430+
closableStreamResponse(EMPTY_DATA_EVENT, closeFirstStream),
431+
streamResponse(EMPTY_DATA_EVENT)
432+
);
433+
AtomicInteger connectionCount = new AtomicInteger();
434+
435+
try (HttpServer server = HttpServer.start(streamHandler)) {
436+
TcpHandler forwardToServer = TcpHandlers.forwardToPort(server.getPort());
437+
TcpHandler countConnections = socket -> {
438+
connectionCount.incrementAndGet();
439+
forwardToServer.apply(socket);
440+
};
441+
try (TcpServer countingServer = TcpServer.start(countConnections)) {
442+
try (StreamProcessor sp = createStreamProcessor(null, countingServer.getHttpUri())) {
443+
startAndWait(sp);
444+
server.getRecorder().requireRequest();
445+
446+
closeFirstStream.release();
447+
server.getRecorder().requireRequest();
448+
449+
assertThat(connectionCount.get(), equalTo(2));
450+
}
451+
}
452+
}
453+
}
454+
425455
@Test
426456
public void streamInitDiagnosticRecordedOnOpen() throws Exception {
427457
DiagnosticStore acc = basicDiagnosticStore();

‎lib/sdk/server/src/test/java/com/launchdarkly/sdk/server/StreamingSynchronizerImplTest.java‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,16 @@
1212
import com.launchdarkly.testhelpers.httptest.Handlers;
1313
import com.launchdarkly.testhelpers.httptest.HttpServer;
1414
import com.launchdarkly.testhelpers.httptest.RequestInfo;
15+
import com.launchdarkly.testhelpers.tcptest.TcpHandler;
16+
import com.launchdarkly.testhelpers.tcptest.TcpHandlers;
17+
import com.launchdarkly.testhelpers.tcptest.TcpServer;
1518
import org.junit.Test;
1619

1720
import java.net.URI;
1821
import java.time.Duration;
1922
import java.util.concurrent.CompletableFuture;
2023
import java.util.concurrent.TimeUnit;
24+
import java.util.concurrent.atomic.AtomicInteger;
2125

2226
import static com.launchdarkly.sdk.server.ComponentsImpl.toHttpProperties;
2327
import static com.launchdarkly.sdk.server.TestComponents.basicDiagnosticStore;
@@ -358,6 +362,57 @@ public void goodbyeEventInResponse() throws Exception {
358362
}
359363
}
360364

365+
@Test
366+
public void reconnectOpensNewConnection() throws Exception {
367+
String serverIntent = makeEvent("server-intent", "{\"payloads\":[{\"id\":\"payload-1\",\"target\":100,\"intentCode\":\"xfer-full\",\"reason\":\"payload-missing\"}]}");
368+
String payloadTransferred = makeEvent("payload-transferred", "{\"state\":\"(p:payload-1:100)\",\"version\":100}");
369+
370+
// The first stream ends normally. This leaves a connection that the HTTP client could reuse.
371+
try (HttpServer server = HttpServer.start(Handlers.sequential(
372+
Handlers.all(
373+
Handlers.SSE.start(),
374+
Handlers.SSE.event(serverIntent),
375+
Handlers.SSE.event(payloadTransferred)),
376+
Handlers.all(
377+
Handlers.SSE.start(),
378+
Handlers.SSE.event(serverIntent),
379+
Handlers.SSE.event(payloadTransferred),
380+
Handlers.SSE.leaveOpen())))) {
381+
AtomicInteger connectionCount = new AtomicInteger();
382+
TcpHandler forwardToServer = TcpHandlers.forwardToPort(server.getPort());
383+
TcpHandler countConnections = socket -> {
384+
connectionCount.incrementAndGet();
385+
forwardToServer.apply(socket);
386+
};
387+
388+
try (TcpServer countingServer = TcpServer.start(countConnections)) {
389+
HttpProperties httpProperties = toHttpProperties(clientContext("sdk-key", baseConfig().build()).getHttp());
390+
391+
StreamingSynchronizerImpl synchronizer = new StreamingSynchronizerImpl(
392+
httpProperties,
393+
countingServer.getHttpUri(),
394+
"/stream",
395+
testLogger,
396+
mockSelectorSource(),
397+
null,
398+
Duration.ofMillis(100),
399+
Thread.NORM_PRIORITY,
400+
null
401+
);
402+
403+
FDv2SourceResult result = synchronizer.next().get(5, TimeUnit.SECONDS);
404+
assertEquals(SourceResultType.CHANGE_SET, result.getResultType());
405+
406+
server.getRecorder().requireRequest();
407+
server.getRecorder().requireRequest();
408+
409+
assertEquals(2, connectionCount.get());
410+
411+
synchronizer.close();
412+
}
413+
}
414+
}
415+
361416
@Test
362417
public void heartbeatEvent() throws Exception {
363418
String heartbeatEvent = makeEvent("heartbeat", "{}");

0 commit comments

Comments
 (0)