Skip to content

Commit 50513bc

Browse files
committed
fix: address review feedback on close, clamps, and polling test coverage
Complete the start() future when a data source is closed. No failure permanently stops either data source now, so shutdown is the only remaining exit for a caller that waits on that future without a timeout; previously such a caller waited forever, since close() never completed it and the error-path completion went away with the permanent-stop paths. Both processors now complete it in close(), after the OFF status is published so a released thread observes a consistent state. isInitialized() still reports false, so this signals "done waiting" rather than "initialized", and it gives the constructor a fast exit if another thread closes the client during startWaitTime. StreamProcessor's future was a local in start() and unreachable from close(), so it becomes a field as PollingProcessor already had; repeated start() calls consequently share one future, which is how PollingProcessor has always behaved and LDClient only calls start() once. Replace the paired compareTo clamps in PollingStrategy.onFailure with a max() helper. Assign-then-maybe-overwrite twice obscured that both bounds are simply floored at the poll interval, and compareTo at the call site made the operand order hard to read. Behavior is unchanged. Add polling coverage that distinguishes the extended regime from the normal cadence. testUnexpectedHttpErrorKeepsPolling used a 30ms extended initial delay against a 20ms poll interval, so after the wait floor the extended wait landed in 20-30ms and every assertion in it passed whether or not the extended regime engaged -- its comment claimed the waits were observable but nothing observed them. The gap that matters is broader: PollingStrategy is covered in isolation, but the wiring from poll() through onFailure and nextWait into scheduleNext had no coverage at all, which is where a mis-wired classifier call would hide. unexpectedFailureUsesExtendedCadence and oneSuccessfulPollDoesNotLeaveExtended- Cadence assert observed request spacing with a 500ms extended initial delay, giving a 250-500ms wait against a 20ms normal cadence. Both use a lower bound only, so CI slowness cannot fail them. Verified non-vacuous by swapping the injected 401 for a 500: the observed gap drops to 33ms and the assertion fails, which is what the previous test could not detect. Also adds the missing "engaging extended backoff" log assertion, which the streaming counterpart already had.
1 parent a339e6b commit 50513bc

4 files changed

Lines changed: 67 additions & 14 deletions

File tree

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ public void close() throws IOException {
7979
logger.info("Closing LaunchDarkly PollingProcessor");
8080
requestor.close();
8181
dataSourceUpdates.updateStatus(State.OFF, null);
82+
initFuture.complete(null);
8283
}
8384

8485
@Override

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

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -77,14 +77,8 @@ boolean onFailure(FailureClass failureClass) {
7777
if (failureClass == FailureClass.UNEXPECTED && !this.inExtended) {
7878
this.inExtended = true;
7979
this.n = 1;
80-
this.initialDelay = extendedInitialInterval;
81-
if (this.initialDelay.compareTo(normalInterval) < 0) {
82-
this.initialDelay = normalInterval;
83-
}
84-
this.maxDelay = EXTENDED_MAX_DELAY;
85-
if (this.maxDelay.compareTo(normalInterval) < 0) {
86-
this.maxDelay = normalInterval;
87-
}
80+
this.initialDelay = max(extendedInitialInterval, normalInterval);
81+
this.maxDelay = max(EXTENDED_MAX_DELAY, normalInterval);
8882
return true;
8983
}
9084
this.n++;
@@ -138,6 +132,10 @@ Duration nextWait() {
138132
return Duration.ofMillis(waitMs);
139133
}
140134

135+
private static Duration max(Duration a, Duration b) {
136+
return a.compareTo(b) >= 0 ? a : b;
137+
}
138+
141139
// Accessors for observability / testing.
142140

143141
int getN() { return n; }

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,7 @@ final class StreamProcessor implements DataSource {
114114
private final AtomicBoolean closed = new AtomicBoolean(false);
115115
private volatile long esStarted = 0;
116116
private volatile boolean lastStoreUpdateFailed = false;
117+
private final CompletableFuture<Void> initFuture = new CompletableFuture<>();
117118
private final LDLogger logger;
118119

119120
StreamProcessor(
@@ -178,8 +179,6 @@ private void onStoreStatusChanged(DataStoreStatusProvider.Status newStatus) {
178179

179180
@Override
180181
public Future<Void> start() {
181-
final CompletableFuture<Void> initFuture = new CompletableFuture<>();
182-
183182
// Notes about the configuration of the EventSource below:
184183
//
185184
// 1. Setting streamEventData(true) is an optimization to let us read the event's data field directly
@@ -278,6 +277,7 @@ public void close() throws IOException {
278277
es.close();
279278
}
280279
dataSourceUpdates.updateStatus(State.OFF, null);
280+
initFuture.complete(null);
281281
}
282282

283283
@Override

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

Lines changed: 58 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
import static org.hamcrest.MatcherAssert.assertThat;
5050
import static org.hamcrest.Matchers.containsString;
5151
import static org.hamcrest.Matchers.equalTo;
52+
import static org.hamcrest.Matchers.greaterThan;
5253
import static org.junit.Assert.assertEquals;
5354
import static org.junit.Assert.assertFalse;
5455
import static org.junit.Assert.assertNotNull;
@@ -60,6 +61,8 @@ public class PollingProcessorTest extends BaseTest {
6061
private static final String SDK_KEY = "sdk-key";
6162
private static final Duration LENGTHY_INTERVAL = Duration.ofSeconds(60);
6263
private static final Duration BRIEF_INTERVAL = Duration.ofMillis(20);
64+
private static final Duration OBSERVABLE_EXTENDED_INITIAL = Duration.ofMillis(500);
65+
private static final long EXTENDED_GAP_FLOOR_MILLIS = 150L;
6366

6467
private MockDataSourceUpdates dataSourceUpdates;
6568

@@ -294,11 +297,54 @@ public void http500ErrorIsRecoverable() throws Exception {
294297
testRecoverableHttpError(500);
295298
}
296299

300+
@Test
301+
public void unexpectedFailureUsesExtendedCadence() throws Exception {
302+
TestPollHandler handler = new TestPollHandler();
303+
handler.setError(401);
304+
try (HttpServer server = HttpServer.start(handler)) {
305+
try (PollingProcessor pp = makeProcessor(
306+
server.getUri(), BRIEF_INTERVAL, OBSERVABLE_EXTENDED_INITIAL)) {
307+
pp.start();
308+
309+
server.getRecorder().requireRequest(); // poll #1: 401, engages the extended regime
310+
long afterFirst = System.currentTimeMillis();
311+
server.getRecorder().requireRequest(); // poll #2: arrives after an extended-regime wait
312+
long gap = System.currentTimeMillis() - afterFirst;
313+
314+
assertThat("gap between polls should be on the extended curve rather than the "
315+
+ BRIEF_INTERVAL.toMillis() + "ms normal cadence; observed " + gap + "ms",
316+
gap, greaterThan(EXTENDED_GAP_FLOOR_MILLIS));
317+
}
318+
}
319+
}
320+
321+
@Test
322+
public void oneSuccessfulPollDoesNotLeaveExtendedCadence() throws Exception {
323+
TestPollHandler handler = new TestPollHandler();
324+
handler.setError(401);
325+
try (HttpServer server = HttpServer.start(handler)) {
326+
try (PollingProcessor pp = makeProcessor(
327+
server.getUri(), BRIEF_INTERVAL, OBSERVABLE_EXTENDED_INITIAL)) {
328+
pp.start();
329+
330+
server.getRecorder().requireRequest(); // poll #1: 401, engages the extended regime
331+
handler.setError(0); // recover, so poll #2 succeeds
332+
333+
server.getRecorder().requireRequest(); // poll #2: first success
334+
long afterFirstSuccess = System.currentTimeMillis();
335+
server.getRecorder().requireRequest(); // poll #3: the confirming success
336+
long gap = System.currentTimeMillis() - afterFirstSuccess;
337+
338+
assertThat("after a single success the SDK should still be on the extended curve; "
339+
+ "observed " + gap + "ms", gap, greaterThan(EXTENDED_GAP_FLOOR_MILLIS));
340+
}
341+
}
342+
}
343+
297344
private void testUnexpectedHttpErrorKeepsPolling(int statusCode) throws Exception {
298-
// 401 / 403 (and other UNEXPECTED 4xx) engage extended-regime backoff via
299-
// PollingStrategy but never trigger a permanent State.OFF. Use a small
300-
// extendedInitialDelay so the extended-regime waits are observable at ms
301-
// scale rather than the 5-minute production default.
345+
// 401 / 403 (and other UNEXPECTED 4xx) keep polling and never trigger a permanent
346+
// State.OFF. Timing is not asserted here -- see unexpectedFailureUsesExtendedCadence
347+
// for the extended-regime cadence.
302348
TestPollHandler handler = new TestPollHandler();
303349
handler.setError(statusCode);
304350
Duration extendedInitial = Duration.ofMillis(30);
@@ -335,6 +381,14 @@ private void testUnexpectedHttpErrorKeepsPolling(int statusCode) throws Exceptio
335381
}
336382
assertTrue("expected an Error-level SDK log mentioning HTTP error " + statusCode,
337383
sawErrorForStatus);
384+
385+
boolean sawEngagedLog = false;
386+
for (LogCapture.Message m : logCapture.getMessages()) {
387+
if (m.getText().contains("engaging extended backoff")) {
388+
sawEngagedLog = true;
389+
}
390+
}
391+
assertTrue("expected the extended-backoff engagement log", sawEngagedLog);
338392
}
339393
}
340394
});

0 commit comments

Comments
 (0)