fix: publish the hook list safely across threads

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<Hook> 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<Hook> hooks;
 
     public HookRunner(LDLogger logger, List<Hook> 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<Hook> updated = new ArrayList<>(hooks);
+        updated.add(hook);
+        hooks = Collections.unmodifiableList(updated);
     }
 
     public EvaluationDetail<LDValue> withEvaluation(String method, String key, LDContext context, LDValue defaultValue, EvaluationMethod evalMethod) {
+        List<Hook> hooks = this.hooks;
         if (hooks.isEmpty()) {
             return evalMethod.evaluate();
         }
@@ -84,6 +97,10 @@ public EvaluationDetail<LDValue> 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<Hook> 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<Hook> hooks = this.hooks;
         if (hooks.isEmpty()) {
             return;
         }

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<String> stages = new ArrayList<>();
+        Map<String, Object> 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<LDValue> 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<String, Object> beforeEvaluation(EvaluationSeriesContext seriesContext, Map<String, Object> seriesData) {
+                                evaluationsObserved.incrementAndGet();
+                                return seriesData;
+                            }
+                        });
+                    }
+                } catch (InterruptedException e) {
+                    Thread.currentThread().interrupt();
+                } finally {
+                    finished.countDown();
+                }
+            }).start();
+        }
+        startLine.countDown();
+        assertTrue(finished.await(10, TimeUnit.SECONDS));
+
+        EvaluationDetail<LDValue> 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";