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 ada3b69b68..d555069555 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 @@ public class GossipLinkStore @Override public Set getEgressLinks(ConnectPoint src) { Set 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 + // 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 @@ public class GossipLinkStore @Override public Set getIngressLinks(ConnectPoint dst) { Set 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 7d58f12f9f..ac819ca356 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 @@ public class SimpleLinkStore @Override public Set getEgressLinks(ConnectPoint src) { Set 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 @@ public class SimpleLinkStore @Override public Set getIngressLinks(ConnectPoint dst) { Set 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;