Fix proposer config refresh (Consensys#7469)

* fix proposer config refresh * improve unit test
courtneyeh · Aug 30, 2023 · d4059e0 · d4059e0
1 parent e89031e
commit d4059e0
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 36 deletions.
diff --git a/...ava/tech/pegasys/teku/validator/client/proposerconfig/AbstractProposerConfigProvider.java b/...ava/tech/pegasys/teku/validator/client/proposerconfig/AbstractProposerConfigProvider.java
@@ -35,7 +35,8 @@ public abstract class AbstractProposerConfigProvider implements ProposerConfigPr
   private final TimeProvider timeProvider;
   private Optional<ProposerConfig> lastProposerConfig = Optional.empty();
   private UInt64 lastProposerConfigTimeStamp = UInt64.ZERO;
-  private Optional<SafeFuture<Optional<ProposerConfig>>> futureProposerConfig = Optional.empty();
+  private SafeFuture<Optional<ProposerConfig>> futureProposerConfig =
+      SafeFuture.completedFuture(Optional.empty());
 
   AbstractProposerConfigProvider(
       final AsyncRunner asyncRunner,
@@ -54,9 +55,9 @@ public synchronized SafeFuture<Optional<ProposerConfig>> getProposerConfig() {
       return SafeFuture.completedFuture(lastProposerConfig);
     }
 
-    if (futureProposerConfig.isPresent()) {
+    if (!futureProposerConfig.isDone()) {
       // a proposer config reload is in progress, use that as result
-      return futureProposerConfig.get();
+      return futureProposerConfig;
     }
 
     if (lastProposerConfig.isPresent()
@@ -67,39 +68,33 @@ public synchronized SafeFuture<Optional<ProposerConfig>> getProposerConfig() {
     }
 
     futureProposerConfig =
-        Optional.of(
-            asyncRunner
-                .runAsync(
-                    () -> {
-                      lastProposerConfig = Optional.of(internalGetProposerConfig());
-                      lastProposerConfigTimeStamp = timeProvider.getTimeInSeconds();
-                      return lastProposerConfig;
-                    })
-                .orTimeout(30, TimeUnit.SECONDS)
-                .exceptionally(
-                    throwable -> {
-                      if (lastProposerConfig.isPresent()) {
-                        LOG.warn(
-                            "An error occurred while obtaining config, providing last loaded config",
-                            throwable);
-                        return lastProposerConfig;
-                      }
-                      throw new RuntimeException(
-                          "An error occurred while obtaining config and there is no previously loaded config",
-                          throwable);
-                    })
-                .thenPeek(
-                    proposerConfig ->
-                        LOG.info(
-                            "Proposer config successfully loaded. It contains the default configuration and {} specific configuration(s).",
-                            proposerConfig.orElseThrow().getNumberOfProposerConfigs()))
-                .alwaysRun(
-                    () -> {
-                      synchronized (this) {
-                        futureProposerConfig = Optional.empty();
-                      }
-                    }));
-    return futureProposerConfig.get();
+        asyncRunner
+            .runAsync(this::internalGetProposerConfig)
+            .orTimeout(30, TimeUnit.SECONDS)
+            .thenApply(this::updateProposerConfig)
+            .exceptionally(this::handleException);
+
+    return futureProposerConfig;
+  }
+
+  private synchronized Optional<ProposerConfig> handleException(final Throwable throwable) {
+    if (lastProposerConfig.isPresent()) {
+      LOG.warn("An error occurred while obtaining config, providing last loaded config", throwable);
+      return lastProposerConfig;
+    }
+    throw new RuntimeException(
+        "An error occurred while obtaining config and there is no previously loaded config",
+        throwable);
+  }
+
+  private synchronized Optional<ProposerConfig> updateProposerConfig(
+      final ProposerConfig proposerConfig) {
+    lastProposerConfig = Optional.of(proposerConfig);
+    lastProposerConfigTimeStamp = timeProvider.getTimeInSeconds();
+    LOG.info(
+        "Proposer config successfully loaded. It contains the default configuration and {} specific configuration(s).",
+        proposerConfig.getNumberOfProposerConfigs());
+    return lastProposerConfig;
   }
 
   protected abstract ProposerConfig internalGetProposerConfig();

diff --git a/...st/java/tech/pegasys/teku/validator/client/proposerconfig/ProposerConfigProviderTest.java b/...st/java/tech/pegasys/teku/validator/client/proposerconfig/ProposerConfigProviderTest.java
@@ -14,6 +14,7 @@
 package tech.pegasys.teku.validator.client.proposerconfig;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -94,12 +95,14 @@ void getProposerConfig_onErrorShouldReturnLastConfigWhenLastConfigAvailable() {
 
     assertThat(futureMaybeConfig).isCompletedWithValue(Optional.of(proposerConfigA));
 
+    timeProvider.advanceTimeBySeconds(LAST_PROPOSER_CONFIG_VALIDITY_PERIOD + 10);
     futureMaybeConfig = proposerConfigProvider.getProposerConfig();
 
     when(proposerConfigLoader.getProposerConfig(sourceUrl))
         .thenThrow(new RuntimeException("error"));
     asyncRunner.executeQueuedActions();
 
+    verify(proposerConfigLoader, times(2)).getProposerConfig(any());
     assertThat(futureMaybeConfig).isCompletedWithValue(Optional.of(proposerConfigA));
   }