diff --git a/changelog/@unreleased/pr-632.v2.yml b/changelog/@unreleased/pr-632.v2.yml new file mode 100644 index 00000000..9fed9066 --- /dev/null +++ b/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 diff --git a/refreshable/src/main/java/com/palantir/refreshable/DefaultRefreshable.java b/refreshable/src/main/java/com/palantir/refreshable/DefaultRefreshable.java index d883e093..2032166c 100644 --- a/refreshable/src/main/java/com/palantir/refreshable/DefaultRefreshable.java +++ b/refreshable/src/main/java/com/palantir/refreshable/DefaultRefreshable.java @@ -132,7 +132,7 @@ public Disposable subscribe(Consumer throwingSubscriber) { SideEffectSubscriber 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 subscriber) { + private Disposable subscribeToSelf(Consumer 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 Refreshable map(Function function) { DefaultRefreshable child = createChild(initialChildValue); MapSubscriber 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 { diff --git a/refreshable/src/test/java/com/palantir/refreshable/RefreshableTest.java b/refreshable/src/test/java/com/palantir/refreshable/RefreshableTest.java index 0ffd9360..f20eb74a 100644 --- a/refreshable/src/test/java/com/palantir/refreshable/RefreshableTest.java +++ b/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 ref = Refreshable.create(new AtomicInteger()); + assertThat(ref.get()).hasValue(0); + Refreshable mapped = ref.map(AtomicInteger::incrementAndGet); + assertThat(mapped.get()).isEqualTo(1); + assertThat(ref.get()).hasValue(1); + } + + @Test + public void testMap_throws() { + SettableRefreshable ref = Refreshable.create(""); + RuntimeException thrown = new RuntimeException(); + assertThatThrownBy(() -> ref.map(_ignored -> { + throw thrown; + })) + .isSameAs(thrown); + } + @Test public void testSubscribe() throws Exception { refreshable.subscribe(consumer);