Fix concurrent access of node permissioning allow list (hyperledger#7920

) Signed-off-by: Bhanu Pulluri <[email protected]> Co-authored-by: Bhanu Pulluri <[email protected]> Co-authored-by: Sally MacFarlane <[email protected]>
Consensys · Nov 25, 2024 · 0e908d2 · 0e908d2
1 parent f64c147
commit 0e908d2
Show file tree

Hide file tree

Showing 2 changed files with 53 additions and 1 deletion.
diff --git a/...a/org/hyperledger/besu/ethereum/permissioning/NodeLocalConfigPermissioningController.java b/...a/org/hyperledger/besu/ethereum/permissioning/NodeLocalConfigPermissioningController.java
@@ -32,6 +32,7 @@
 import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.function.Consumer;
 import java.util.stream.Collectors;
 
@@ -48,7 +49,7 @@ public class NodeLocalConfigPermissioningController implements NodeConnectionPer
   private LocalPermissioningConfiguration configuration;
   private final List<EnodeURL> fixedNodes;
   private final Bytes localNodeId;
-  private final List<EnodeURL> nodesAllowlist = new ArrayList<>();
+  private final List<EnodeURL> nodesAllowlist = new CopyOnWriteArrayList<>();
   private final AllowlistPersistor allowlistPersistor;
   private final Subscribers<Consumer<NodeAllowlistUpdatedEvent>> nodeAllowlistUpdatedObservers =
       Subscribers.create();

diff --git a/...g/hyperledger/besu/ethereum/permissioning/NodeLocalConfigPermissioningControllerTest.java b/...g/hyperledger/besu/ethereum/permissioning/NodeLocalConfigPermissioningControllerTest.java
@@ -46,6 +46,7 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.Consumer;
 
 import com.google.common.collect.Lists;
@@ -316,6 +317,56 @@ public void whenCheckingIfNodeIsPermittedOrderDoesNotMatter() {
         .isTrue();
   }
 
+  @Test
+  public void whenCallingIsPermittedAndRemovingEntryInAnotherThreadShouldNotThrowException()
+      throws InterruptedException {
+    // Add a node to the allowlist
+    controller.addNodes(Lists.newArrayList(enode1));
+
+    // Atomic flag to detect exceptions
+    AtomicBoolean exceptionOccurred = new AtomicBoolean(false);
+
+    // Create a thread to call isPermitted
+    Thread isPermittedThread =
+        new Thread(
+            () -> {
+              try {
+                for (int i = 0; i < 1000; i++) {
+                  controller.isPermitted(enode1);
+                }
+              } catch (Exception e) {
+                exceptionOccurred.set(true);
+                e.printStackTrace();
+              }
+            });
+
+    // Create a thread to modify the allowlist
+    Thread modifyAllowlistThread =
+        new Thread(
+            () -> {
+              try {
+                for (int i = 0; i < 1000; i++) {
+                  controller.removeNodes(Lists.newArrayList(enode1));
+                  controller.addNodes(Lists.newArrayList(enode1));
+                }
+              } catch (Exception e) {
+                exceptionOccurred.set(true);
+                e.printStackTrace();
+              }
+            });
+
+    // Start both threads
+    isPermittedThread.start();
+    modifyAllowlistThread.start();
+
+    // Wait for both threads to complete
+    isPermittedThread.join();
+    modifyAllowlistThread.join();
+
+    // Assert no exceptions were thrown
+    assert (!exceptionOccurred.get());
+  }
+
   @Test
   public void stateShouldRevertIfAllowlistPersistFails()
       throws IOException, AllowlistFileSyncException {