refreshable.map calls the map function exactly once (#632)

refreshable.map calls the map function exactly once

changelog/@unreleased/pr-632.v2.yml

@@ -0,0 +1,5 @@
+type: improvement
+improvement:
+  description: refreshable.map calls the map function exactly once
+  links:
+  - https://github.com/palantir/refreshable/pull/632

refreshable/src/main/java/com/palantir/refreshable/DefaultRefreshable.java

@@ -132,7 +132,7 @@ public Disposable subscribe(Consumer<? super T> throwingSubscriber) {
             SideEffectSubscriber<? super T> trackedSubscriber =
                     rootSubscriberTracker.newSideEffectSubscriber(throwingSubscriber, this);
 
-            Disposable disposable = subscribeToSelf(trackedSubscriber);
+            Disposable disposable = subscribeToSelf(trackedSubscriber, true);
             return new SubscribeDisposable(disposable, rootSubscriberTracker, trackedSubscriber);
         } finally {
             readLock.unlock();
@@ -161,10 +161,12 @@ public void dispose() {
     }
 
     @GuardedBy("readLock")
-    private Disposable subscribeToSelf(Consumer<? super T> subscriber) {
+    private Disposable subscribeToSelf(Consumer<? super T> subscriber, boolean updateSubscriber) {
         preSubscribeLogging();
         orderedSubscribers.add(subscriber);
-        subscriber.accept(current);
+        if (updateSubscriber) {
+            subscriber.accept(current);
+        }
         return new DefaultDisposable(orderedSubscribers, subscriber);
     }
 
@@ -210,7 +212,9 @@ public <R> Refreshable<R> map(Function<? super T, R> function) {
             DefaultRefreshable<R> child = createChild(initialChildValue);
 
             MapSubscriber<? super T, R> mapSubscriber = new MapSubscriber<>(function, child);
-            Disposable cleanUp = subscribeToSelf(mapSubscriber);
+            // Do not update the subscriber here because we've just computed the value while
+            // holding readLock above to ensure bad functions throw on 'map' invocation.
+            Disposable cleanUp = subscribeToSelf(mapSubscriber, false);
             REFRESHABLE_CLEANER.register(child, cleanUp::dispose);
             return child;
         } finally {

refreshable/src/test/java/com/palantir/refreshable/RefreshableTest.java

@@ -114,6 +114,25 @@ public void testMap_noEventsWhenDerivedValueUnchanged() throws Exception {
         verifyNoMoreInteractions(consumer, derivedConsumer);
     }
 
+    @Test
+    public void testMap_calledOnce() {
+        SettableRefreshable<AtomicInteger> ref = Refreshable.create(new AtomicInteger());
+        assertThat(ref.get()).hasValue(0);
+        Refreshable<Integer> mapped = ref.map(AtomicInteger::incrementAndGet);
+        assertThat(mapped.get()).isEqualTo(1);
+        assertThat(ref.get()).hasValue(1);
+    }
+
+    @Test
+    public void testMap_throws() {
+        SettableRefreshable<String> ref = Refreshable.create("");
+        RuntimeException thrown = new RuntimeException();
+        assertThatThrownBy(() -> ref.map(_ignored -> {
+                    throw thrown;
+                }))
+                .isSameAs(thrown);
+    }
+
     @Test
     public void testSubscribe() throws Exception {
         refreshable.subscribe(consumer);