From bd0de9464c97f08a4dd34fd11cfe5923c36bd289 Mon Sep 17 00:00:00 2001 From: Carpenter Lee Date: Mon, 29 Oct 2018 20:19:29 +0800 Subject: [PATCH] Avoid ConcurrentModificationException when getting machine map in AppInfo (#205) - Avoid ConcurrentModificationException when `getMachines()` by deep clone the machine map - Enhance the code to avoid potential ConcurrentModificationException - Add test cases --- sentinel-dashboard/pom.xml | 5 ++ .../sentinel/dashboard/discovery/AppInfo.java | 16 ++++-- .../dashboard/discovery/MachineInfo.java | 26 +++------- .../dashboard/discovery/AppInfoTest.java | 52 +++++++++++++++++++ 4 files changed, 75 insertions(+), 24 deletions(-) create mode 100644 sentinel-dashboard/src/test/java/com/taobao/csp/sentinel/dashboard/discovery/AppInfoTest.java diff --git a/sentinel-dashboard/pom.xml b/sentinel-dashboard/pom.xml index 30742426..0092d05e 100755 --- a/sentinel-dashboard/pom.xml +++ b/sentinel-dashboard/pom.xml @@ -99,6 +99,11 @@ com.alibaba fastjson + + junit + junit + test + diff --git a/sentinel-dashboard/src/main/java/com/taobao/csp/sentinel/dashboard/discovery/AppInfo.java b/sentinel-dashboard/src/main/java/com/taobao/csp/sentinel/dashboard/discovery/AppInfo.java index 614b42b4..ff9a5262 100755 --- a/sentinel-dashboard/src/main/java/com/taobao/csp/sentinel/dashboard/discovery/AppInfo.java +++ b/sentinel-dashboard/src/main/java/com/taobao/csp/sentinel/dashboard/discovery/AppInfo.java @@ -15,15 +15,16 @@ */ package com.taobao.csp.sentinel.dashboard.discovery; +import java.util.HashSet; import java.util.Optional; import java.util.Set; -import java.util.TreeSet; +import java.util.concurrent.ConcurrentHashMap; public class AppInfo { private String app = ""; - private Set machines = new TreeSet(); + private Set machines = ConcurrentHashMap.newKeySet(); public AppInfo() { } @@ -40,8 +41,13 @@ public class AppInfo { this.app = app; } - public synchronized Set getMachines() { - return machines; + /** + * Get the current machines. + * + * @return a new copy of the current machines. + */ + public Set getMachines() { + return new HashSet<>(machines); } @Override @@ -49,7 +55,7 @@ public class AppInfo { return "AppInfo{" + "app='" + app + ", machines=" + machines + '}'; } - public synchronized boolean addMachine(MachineInfo machineInfo) { + public boolean addMachine(MachineInfo machineInfo) { machines.remove(machineInfo); return machines.add(machineInfo); } diff --git a/sentinel-dashboard/src/main/java/com/taobao/csp/sentinel/dashboard/discovery/MachineInfo.java b/sentinel-dashboard/src/main/java/com/taobao/csp/sentinel/dashboard/discovery/MachineInfo.java index dc2723b7..67db4f4b 100755 --- a/sentinel-dashboard/src/main/java/com/taobao/csp/sentinel/dashboard/discovery/MachineInfo.java +++ b/sentinel-dashboard/src/main/java/com/taobao/csp/sentinel/dashboard/discovery/MachineInfo.java @@ -16,6 +16,7 @@ package com.taobao.csp.sentinel.dashboard.discovery; import java.util.Date; +import java.util.Objects; import com.alibaba.csp.sentinel.util.StringUtil; @@ -117,29 +118,16 @@ public class MachineInfo implements Comparable { @Override public boolean equals(Object o) { - if (this == o) { - return true; - } - if (!(o instanceof MachineInfo)) { - return false; - } - + if (this == o) { return true; } + if (!(o instanceof MachineInfo)) { return false; } MachineInfo that = (MachineInfo)o; - - if (app != null ? !app.equals(that.app) : that.app != null) { - return false; - } - if (hostname != null ? !hostname.equals(that.hostname) : that.hostname != null) { - return false; - } - return ip != null ? ip.equals(that.ip) : that.ip == null; + return Objects.equals(app, that.app) && + Objects.equals(ip, that.ip) && + Objects.equals(port, that.port); } @Override public int hashCode() { - int result = app != null ? app.hashCode() : 0; - result = 31 * result + (hostname != null ? hostname.hashCode() : 0); - result = 31 * result + (ip != null ? ip.hashCode() : 0); - return result; + return Objects.hash(app, ip, port); } } diff --git a/sentinel-dashboard/src/test/java/com/taobao/csp/sentinel/dashboard/discovery/AppInfoTest.java b/sentinel-dashboard/src/test/java/com/taobao/csp/sentinel/dashboard/discovery/AppInfoTest.java new file mode 100644 index 00000000..a92c8053 --- /dev/null +++ b/sentinel-dashboard/src/test/java/com/taobao/csp/sentinel/dashboard/discovery/AppInfoTest.java @@ -0,0 +1,52 @@ +package com.taobao.csp.sentinel.dashboard.discovery; + +import java.util.ConcurrentModificationException; +import java.util.Set; + +import org.junit.Test; + +import static org.junit.Assert.*; + +public class AppInfoTest { + @Test + public void testConcurrentGetMachines() throws Exception { + AppInfo appInfo = new AppInfo("testApp"); + appInfo.addMachine(genMachineInfo("hostName1", "10.18.129.91")); + appInfo.addMachine(genMachineInfo("hostName2", "10.18.129.92")); + Set machines = appInfo.getMachines(); + new Thread(() -> { + try { + for (MachineInfo m : machines) { + System.out.println(m); + try { + Thread.sleep(200); + } catch (InterruptedException e) { + } + } + } catch (ConcurrentModificationException e) { + e.printStackTrace(); + assertTrue(false); + } + + }).start(); + Thread.sleep(100); + try { + appInfo.addMachine(genMachineInfo("hostName3", "10.18.129.93")); + } catch (ConcurrentModificationException e) { + e.printStackTrace(); + assertTrue(false); + } + Thread.sleep(1000); + } + + private MachineInfo genMachineInfo(String hostName, String ip) { + MachineInfo machine = new MachineInfo(); + machine.setApp("testApp"); + machine.setHostname(hostName); + machine.setIp(ip); + machine.setPort(8719); + machine.setVersion(String.valueOf(System.currentTimeMillis())); + return machine; + } + +} \ No newline at end of file