From 048e17c62be2cfb92d4dafef0bddb603716890bc Mon Sep 17 00:00:00 2001
From: Jon Chambers <jon@signal.org>
Date: Wed, 24 Nov 2021 17:45:33 -0500
Subject: [PATCH] Use a memoizing supplier instead of a looping thread to cache
 remote config entries

---
 .../textsecuregcm/WhisperServerService.java   |  1 -
 .../storage/RemoteConfigsManager.java         | 63 +++----------------
 .../storage/RemoteConfigsManagerTest.java     |  9 +--
 3 files changed, 10 insertions(+), 63 deletions(-)

diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java
index c41ff2136..d7b678b56 100644
--- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java
+++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java
@@ -593,7 +593,6 @@ public class WhisperServerService extends Application<WhisperServerConfiguration
     environment.lifecycle().manage(accountDatabaseCrawler);
     environment.lifecycle().manage(directoryReconciliationAccountDatabaseCrawler);
     environment.lifecycle().manage(deletedAccountsTableCrawler);
-    environment.lifecycle().manage(remoteConfigsManager);
     environment.lifecycle().manage(messagesCache);
     environment.lifecycle().manage(messagePersister);
     environment.lifecycle().manage(clientPresenceManager);
diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/RemoteConfigsManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/RemoteConfigsManager.java
index a6dcfe6dd..267cfc275 100644
--- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/RemoteConfigsManager.java
+++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/RemoteConfigsManager.java
@@ -5,70 +5,26 @@
 
 package org.whispersystems.textsecuregcm.storage;
 
-import com.google.common.annotations.VisibleForTesting;
-import io.dropwizard.lifecycle.Managed;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-import org.whispersystems.textsecuregcm.util.Util;
-
-import java.util.LinkedList;
+import com.google.common.base.Suppliers;
 import java.util.List;
 import java.util.concurrent.TimeUnit;
-import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
 
-public class RemoteConfigsManager implements Managed {
-
-  private final Logger logger = LoggerFactory.getLogger(RemoteConfigsManager.class);
+public class RemoteConfigsManager {
 
   private final RemoteConfigs remoteConfigs;
-  private final long          sleepInterval;
 
-  private final AtomicReference<List<RemoteConfig>> cachedConfigs = new AtomicReference<>(new LinkedList<>());
+  private final Supplier<List<RemoteConfig>> remoteConfigSupplier;
 
   public RemoteConfigsManager(RemoteConfigs remoteConfigs) {
-    this(remoteConfigs, TimeUnit.SECONDS.toMillis(10));
-  }
-
-  @VisibleForTesting
-  public RemoteConfigsManager(RemoteConfigs remoteConfigs, long sleepInterval) {
     this.remoteConfigs = remoteConfigs;
-    this.sleepInterval = sleepInterval;
-  }
 
-  @Override
-  public void start() {
-    refreshCache();
-
-    new Thread(() -> {
-      while (true) {
-        try {
-          refreshCache();
-        } catch (Throwable t) {
-          logger.warn("Error updating remote configs cache", t);
-        }
-
-        Util.sleep(sleepInterval);
-      }
-    }).start();
-  }
-
-  private void refreshCache() {
-    this.cachedConfigs.set(remoteConfigs.getAll());
-
-    synchronized (this.cachedConfigs) {
-      this.cachedConfigs.notifyAll();
-    }
-  }
-
-  @VisibleForTesting
-  void waitForCacheRefresh() throws InterruptedException {
-    synchronized (this.cachedConfigs) {
-      this.cachedConfigs.wait();
-    }
+    remoteConfigSupplier =
+        Suppliers.memoizeWithExpiration(remoteConfigs::getAll, 10, TimeUnit.SECONDS);
   }
 
   public List<RemoteConfig> getAll() {
-    return cachedConfigs.get();
+    return remoteConfigSupplier.get();
   }
 
   public void set(RemoteConfig config) {
@@ -78,9 +34,4 @@ public class RemoteConfigsManager implements Managed {
   public void delete(String name) {
     remoteConfigs.delete(name);
   }
-
-  @Override
-  public void stop() throws Exception {
-
-  }
 }
diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/RemoteConfigsManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/RemoteConfigsManagerTest.java
index c712f3749..423866f18 100644
--- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/RemoteConfigsManagerTest.java
+++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/RemoteConfigsManagerTest.java
@@ -29,21 +29,18 @@ public class RemoteConfigsManagerTest {
 
   @Before
   public void setup() {
-    RemoteConfigs remoteConfigs = new RemoteConfigs(new FaultTolerantDatabase("remote_configs-test", Jdbi.create(db.getTestDatabase()), new CircuitBreakerConfiguration()));
-    this.remoteConfigs = new RemoteConfigsManager(remoteConfigs, 500);
-    this.remoteConfigs.start();
+    this.remoteConfigs = new RemoteConfigsManager(new RemoteConfigs(
+        new FaultTolerantDatabase("remote_configs-test", Jdbi.create(db.getTestDatabase()), new CircuitBreakerConfiguration())));
   }
 
   @Test
-  public void testUpdate() throws InterruptedException {
+  public void testUpdate() {
     remoteConfigs.set(new RemoteConfig("android.stickers", 50, Set.of(AuthHelper.VALID_UUID), "FALSE", "TRUE", null));
     remoteConfigs.set(new RemoteConfig("value.sometimes", 50, Set.of(), "bar", "baz", null));
     remoteConfigs.set(new RemoteConfig("ios.stickers", 50, Set.of(), "FALSE", "TRUE", null));
     remoteConfigs.set(new RemoteConfig("ios.stickers", 75, Set.of(), "FALSE", "TRUE", null));
     remoteConfigs.set(new RemoteConfig("value.sometimes", 25, Set.of(AuthHelper.VALID_UUID), "abc", "def", null));
 
-    remoteConfigs.waitForCacheRefresh();
-
     List<RemoteConfig> results = remoteConfigs.getAll();
 
     assertThat(results.size()).isEqualTo(3);