diff --git a/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/HookRunner.java b/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/HookRunner.java index b05ae07e..4d9cf050 100644 --- a/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/HookRunner.java +++ b/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/HookRunner.java @@ -29,11 +29,16 @@ public interface AfterIdentifyMethod { private static final String UNKNOWN_HOOK_NAME = "unknown hook"; private final LDLogger logger; - private final List hooks = new ArrayList<>(); + /** + * The hooks to run, which is replaced rather than modified so that a caller of {@link #addHook(Hook)} on one thread + * cannot be seen half way by a series running on another. Every method that runs hooks reads this once into a local + * and works from that, so the hooks a series ends with are the hooks it began with. + */ + private volatile List hooks; public HookRunner(LDLogger logger, List initialHooks) { this.logger = logger; - this.hooks.addAll(initialHooks); + this.hooks = Collections.unmodifiableList(new ArrayList<>(initialHooks)); } private String getHookName(Hook hook) { @@ -46,11 +51,19 @@ private String getHookName(Hook hook) { } } - public void addHook(Hook hook) { - hooks.add(hook); + /** + * Adds a hook, which the next series to begin will run. A series already under way runs the hooks it began with. + * + * @param hook the hook to add + */ + public synchronized void addHook(Hook hook) { + List updated = new ArrayList<>(hooks); + updated.add(hook); + hooks = Collections.unmodifiableList(updated); } public EvaluationDetail withEvaluation(String method, String key, LDContext context, LDValue defaultValue, EvaluationMethod evalMethod) { + List hooks = this.hooks; if (hooks.isEmpty()) { return evalMethod.evaluate(); } @@ -84,6 +97,10 @@ public EvaluationDetail withEvaluation(String method, String key, LDCon } public AfterIdentifyMethod identify(LDContext context, Integer timeout) { + // The returned method runs when the identify completes, which is a round trip later, so it closes over these + // hooks rather than reading the field again: otherwise a hook added in between would be given an "after" stage + // for a series whose "before" stage it was never in. + List hooks = this.hooks; if (hooks.isEmpty()) { return (IdentifySeriesResult result) -> {}; } @@ -115,6 +132,7 @@ public AfterIdentifyMethod identify(LDContext context, Integer timeout) { } public void afterTrack(String key, LDContext context, LDValue data, Double metricValue) { + List hooks = this.hooks; if (hooks.isEmpty()) { return; } diff --git a/launchdarkly-android-client-sdk/src/test/java/com/launchdarkly/sdk/android/HookRunnerTest.java b/launchdarkly-android-client-sdk/src/test/java/com/launchdarkly/sdk/android/HookRunnerTest.java index af1fdaf1..b4ad5c83 100644 --- a/launchdarkly-android-client-sdk/src/test/java/com/launchdarkly/sdk/android/HookRunnerTest.java +++ b/launchdarkly-android-client-sdk/src/test/java/com/launchdarkly/sdk/android/HookRunnerTest.java @@ -5,6 +5,7 @@ import static org.easymock.EasyMock.expectLastCall; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; import com.launchdarkly.sdk.EvaluationDetail; import com.launchdarkly.sdk.EvaluationReason; @@ -26,6 +27,9 @@ import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; public class HookRunnerTest extends EasyMockSupport { private HookRunner hookRunner; @@ -392,6 +396,98 @@ public void executesAfterTrackHookStagesInTheCorrectOrder() { logging.assertNothingLogged(); } + @Test + public void givesAnEvaluationTheHooksItBeganWith() { + List stages = new ArrayList<>(); + Map seriesData = Collections.unmodifiableMap(Collections.emptyMap()); + + Hook addedDuring = mock(Hook.class); + expect(addedDuring.beforeEvaluation(anyObject(), anyObject())).andStubAnswer(() -> { stages.add("added:before"); return seriesData; }); + expect(addedDuring.afterEvaluation(anyObject(), anyObject(), anyObject())).andStubAnswer(() -> { stages.add("added:after"); return seriesData; }); + expect(testHook.beforeEvaluation(anyObject(), anyObject())).andStubAnswer(() -> { + stages.add("first:before"); + hookRunner.addHook(addedDuring); + return seriesData; + }); + expect(testHook.afterEvaluation(anyObject(), anyObject(), anyObject())).andStubAnswer(() -> { stages.add("first:after"); return seriesData; }); + replayAll(); + + EvaluationDetail evaluationResult = EvaluationDetail.fromValue(LDValue.of(true), 1, EvaluationReason.off()); + HookRunner.EvaluationMethod evaluationMethod = () -> evaluationResult; + LDContext context = LDContext.create("user-123"); + hookRunner.withEvaluation("testMethod", "test-flag", context, LDValue.of(false), evaluationMethod); + hookRunner.withEvaluation("testMethod", "test-flag", context, LDValue.of(false), evaluationMethod); + + // A hook registered part way through an evaluation runs from the next one, rather than joining a series whose + // earlier stages it was not in. + assertEquals(List.of("first:before", "first:after", + "first:before", "added:before", "added:after", "first:after"), stages); + logging.assertNothingLogged(); + } + + @Test + public void givesAnIdentifyTheHooksItBeganWith() { + LDContext context = LDContext.create("user-123"); + Integer timeout = 10; + + IdentifySeriesResult identifyResult = new IdentifySeriesResult(IdentifySeriesResult.IdentifySeriesStatus.COMPLETED); + IdentifySeriesContext seriesContext = new IdentifySeriesContext(context, timeout); + Hook addedDuring = mock(Hook.class); + + expect(testHook.beforeIdentify(seriesContext, Collections.emptyMap())).andReturn(Collections.unmodifiableMap(Collections.emptyMap())); + expect(testHook.afterIdentify(seriesContext, Collections.emptyMap(), identifyResult)).andReturn(Collections.unmodifiableMap(Collections.emptyMap())); + replayAll(); + + // An identify's two stages are separated by a round trip, which is time enough for an application to register a + // hook. The new hook is left for the next identify: there is no series data to give it for this one. + HookRunner.AfterIdentifyMethod afterIdentifyMethod = hookRunner.identify(context, timeout); + hookRunner.addHook(addedDuring); + afterIdentifyMethod.invoke(identifyResult); + + verifyAll(); + logging.assertNothingLogged(); + } + + @Test + public void addsHooksFromSeveralThreadsWithoutLosingAny() throws InterruptedException { + int threads = 4; + int hooksPerThread = 50; + HookRunner runner = new HookRunner(logging.logger, Collections.emptyList()); + AtomicInteger evaluationsObserved = new AtomicInteger(); + CountDownLatch startLine = new CountDownLatch(1); + CountDownLatch finished = new CountDownLatch(threads); + + for (int i = 0; i < threads; i++) { + new Thread(() -> { + try { + startLine.await(); + for (int j = 0; j < hooksPerThread; j++) { + runner.addHook(new Hook("counting-hook") { + @Override + public Map beforeEvaluation(EvaluationSeriesContext seriesContext, Map seriesData) { + evaluationsObserved.incrementAndGet(); + return seriesData; + } + }); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } finally { + finished.countDown(); + } + }).start(); + } + startLine.countDown(); + assertTrue(finished.await(10, TimeUnit.SECONDS)); + + EvaluationDetail evaluationResult = EvaluationDetail.fromValue(LDValue.of(true), 1, EvaluationReason.off()); + runner.withEvaluation("testMethod", "test-flag", LDContext.create("user-123"), LDValue.of(false), () -> evaluationResult); + + // Registrations racing one another are each kept, rather than one thread's copy of the list overwriting another's. + assertEquals(threads * hooksPerThread, evaluationsObserved.get()); + logging.assertNothingLogged(); + } + @Test public void logsUnknownHookWhenGetMetadataThrows() { String method = "testMethod";