Workaround for ConcurrentModificationException
- Workaround fix for ONOS-1193
- If locking the whole link collection damages the performance,
apply the fix suggested on GossipLinkStore comment.
Change-Id: Idd4d2e5b8e3a50843fe7abd10a615cfbd9a16478
diff --git a/core/store/dist/src/main/java/org/onosproject/store/link/impl/GossipLinkStore.java b/core/store/dist/src/main/java/org/onosproject/store/link/impl/GossipLinkStore.java
index ada3b69..d555069 100644
--- a/core/store/dist/src/main/java/org/onosproject/store/link/impl/GossipLinkStore.java
+++ b/core/store/dist/src/main/java/org/onosproject/store/link/impl/GossipLinkStore.java
@@ -246,13 +246,26 @@
@Override
public Set<Link> getEgressLinks(ConnectPoint src) {
Set<Link> egress = new HashSet<>();
- for (LinkKey linkKey : srcLinks.get(src.deviceId())) {
- if (linkKey.src().equals(src)) {
- Link link = links.get(linkKey);
- if (link != null) {
- egress.add(link);
- } else {
- log.debug("Egress link for {} was null, skipped", linkKey);
+ //
+ // Change `srcLinks` to ConcurrentMap<DeviceId, (Concurrent)Set>
+ // to remove this synchronized block, if we hit performance issue.
+ // SetMultiMap#get returns wrapped collection to provide modifiable-view.
+ // And the wrapped collection is not concurrent access safe.
+ //
+ // Our use case here does not require returned collection to be modifiable,
+ // so the wrapped collection forces us to lock the whole multiset,
+ // for benefit we don't need.
+ //
+ // Same applies to `dstLinks`
+ synchronized (srcLinks) {
+ for (LinkKey linkKey : srcLinks.get(src.deviceId())) {
+ if (linkKey.src().equals(src)) {
+ Link link = links.get(linkKey);
+ if (link != null) {
+ egress.add(link);
+ } else {
+ log.debug("Egress link for {} was null, skipped", linkKey);
+ }
}
}
}
@@ -262,13 +275,15 @@
@Override
public Set<Link> getIngressLinks(ConnectPoint dst) {
Set<Link> ingress = new HashSet<>();
- for (LinkKey linkKey : dstLinks.get(dst.deviceId())) {
- if (linkKey.dst().equals(dst)) {
- Link link = links.get(linkKey);
- if (link != null) {
- ingress.add(link);
- } else {
- log.debug("Ingress link for {} was null, skipped", linkKey);
+ synchronized (dstLinks) {
+ for (LinkKey linkKey : dstLinks.get(dst.deviceId())) {
+ if (linkKey.dst().equals(dst)) {
+ Link link = links.get(linkKey);
+ if (link != null) {
+ ingress.add(link);
+ } else {
+ log.debug("Ingress link for {} was null, skipped", linkKey);
+ }
}
}
}
diff --git a/core/store/trivial/src/main/java/org/onosproject/store/trivial/impl/SimpleLinkStore.java b/core/store/trivial/src/main/java/org/onosproject/store/trivial/impl/SimpleLinkStore.java
index 7d58f12..ac819ca 100644
--- a/core/store/trivial/src/main/java/org/onosproject/store/trivial/impl/SimpleLinkStore.java
+++ b/core/store/trivial/src/main/java/org/onosproject/store/trivial/impl/SimpleLinkStore.java
@@ -146,9 +146,11 @@
@Override
public Set<Link> getEgressLinks(ConnectPoint src) {
Set<Link> egress = new HashSet<>();
- for (LinkKey linkKey : srcLinks.get(src.deviceId())) {
- if (linkKey.src().equals(src)) {
- egress.add(links.get(linkKey));
+ synchronized (srcLinks) {
+ for (LinkKey linkKey : srcLinks.get(src.deviceId())) {
+ if (linkKey.src().equals(src)) {
+ egress.add(links.get(linkKey));
+ }
}
}
return egress;
@@ -157,9 +159,11 @@
@Override
public Set<Link> getIngressLinks(ConnectPoint dst) {
Set<Link> ingress = new HashSet<>();
- for (LinkKey linkKey : dstLinks.get(dst.deviceId())) {
- if (linkKey.dst().equals(dst)) {
- ingress.add(links.get(linkKey));
+ synchronized (dstLinks) {
+ for (LinkKey linkKey : dstLinks.get(dst.deviceId())) {
+ if (linkKey.dst().equals(dst)) {
+ ingress.add(links.get(linkKey));
+ }
}
}
return ingress;