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
Showing
2 changed files
with
39 additions
and
20 deletions
| ... | @@ -246,13 +246,26 @@ public class GossipLinkStore | ... | @@ -246,13 +246,26 @@ public class GossipLinkStore |
| 246 | @Override | 246 | @Override |
| 247 | public Set<Link> getEgressLinks(ConnectPoint src) { | 247 | public Set<Link> getEgressLinks(ConnectPoint src) { |
| 248 | Set<Link> egress = new HashSet<>(); | 248 | Set<Link> egress = new HashSet<>(); |
| 249 | - for (LinkKey linkKey : srcLinks.get(src.deviceId())) { | 249 | + // |
| 250 | - if (linkKey.src().equals(src)) { | 250 | + // Change `srcLinks` to ConcurrentMap<DeviceId, (Concurrent)Set> |
| 251 | - Link link = links.get(linkKey); | 251 | + // to remove this synchronized block, if we hit performance issue. |
| 252 | - if (link != null) { | 252 | + // SetMultiMap#get returns wrapped collection to provide modifiable-view. |
| 253 | - egress.add(link); | 253 | + // And the wrapped collection is not concurrent access safe. |
| 254 | - } else { | 254 | + // |
| 255 | - log.debug("Egress link for {} was null, skipped", linkKey); | 255 | + // Our use case here does not require returned collection to be modifiable, |
| 256 | + // so the wrapped collection forces us to lock the whole multiset, | ||
| 257 | + // for benefit we don't need. | ||
| 258 | + // | ||
| 259 | + // Same applies to `dstLinks` | ||
| 260 | + synchronized (srcLinks) { | ||
| 261 | + for (LinkKey linkKey : srcLinks.get(src.deviceId())) { | ||
| 262 | + if (linkKey.src().equals(src)) { | ||
| 263 | + Link link = links.get(linkKey); | ||
| 264 | + if (link != null) { | ||
| 265 | + egress.add(link); | ||
| 266 | + } else { | ||
| 267 | + log.debug("Egress link for {} was null, skipped", linkKey); | ||
| 268 | + } | ||
| 256 | } | 269 | } |
| 257 | } | 270 | } |
| 258 | } | 271 | } |
| ... | @@ -262,13 +275,15 @@ public class GossipLinkStore | ... | @@ -262,13 +275,15 @@ public class GossipLinkStore |
| 262 | @Override | 275 | @Override |
| 263 | public Set<Link> getIngressLinks(ConnectPoint dst) { | 276 | public Set<Link> getIngressLinks(ConnectPoint dst) { |
| 264 | Set<Link> ingress = new HashSet<>(); | 277 | Set<Link> ingress = new HashSet<>(); |
| 265 | - for (LinkKey linkKey : dstLinks.get(dst.deviceId())) { | 278 | + synchronized (dstLinks) { |
| 266 | - if (linkKey.dst().equals(dst)) { | 279 | + for (LinkKey linkKey : dstLinks.get(dst.deviceId())) { |
| 267 | - Link link = links.get(linkKey); | 280 | + if (linkKey.dst().equals(dst)) { |
| 268 | - if (link != null) { | 281 | + Link link = links.get(linkKey); |
| 269 | - ingress.add(link); | 282 | + if (link != null) { |
| 270 | - } else { | 283 | + ingress.add(link); |
| 271 | - log.debug("Ingress link for {} was null, skipped", linkKey); | 284 | + } else { |
| 285 | + log.debug("Ingress link for {} was null, skipped", linkKey); | ||
| 286 | + } | ||
| 272 | } | 287 | } |
| 273 | } | 288 | } |
| 274 | } | 289 | } | ... | ... |
| ... | @@ -146,9 +146,11 @@ public class SimpleLinkStore | ... | @@ -146,9 +146,11 @@ public class SimpleLinkStore |
| 146 | @Override | 146 | @Override |
| 147 | public Set<Link> getEgressLinks(ConnectPoint src) { | 147 | public Set<Link> getEgressLinks(ConnectPoint src) { |
| 148 | Set<Link> egress = new HashSet<>(); | 148 | Set<Link> egress = new HashSet<>(); |
| 149 | - for (LinkKey linkKey : srcLinks.get(src.deviceId())) { | 149 | + synchronized (srcLinks) { |
| 150 | - if (linkKey.src().equals(src)) { | 150 | + for (LinkKey linkKey : srcLinks.get(src.deviceId())) { |
| 151 | - egress.add(links.get(linkKey)); | 151 | + if (linkKey.src().equals(src)) { |
| 152 | + egress.add(links.get(linkKey)); | ||
| 153 | + } | ||
| 152 | } | 154 | } |
| 153 | } | 155 | } |
| 154 | return egress; | 156 | return egress; |
| ... | @@ -157,9 +159,11 @@ public class SimpleLinkStore | ... | @@ -157,9 +159,11 @@ public class SimpleLinkStore |
| 157 | @Override | 159 | @Override |
| 158 | public Set<Link> getIngressLinks(ConnectPoint dst) { | 160 | public Set<Link> getIngressLinks(ConnectPoint dst) { |
| 159 | Set<Link> ingress = new HashSet<>(); | 161 | Set<Link> ingress = new HashSet<>(); |
| 160 | - for (LinkKey linkKey : dstLinks.get(dst.deviceId())) { | 162 | + synchronized (dstLinks) { |
| 161 | - if (linkKey.dst().equals(dst)) { | 163 | + for (LinkKey linkKey : dstLinks.get(dst.deviceId())) { |
| 162 | - ingress.add(links.get(linkKey)); | 164 | + if (linkKey.dst().equals(dst)) { |
| 165 | + ingress.add(links.get(linkKey)); | ||
| 166 | + } | ||
| 163 | } | 167 | } |
| 164 | } | 168 | } |
| 165 | return ingress; | 169 | return ingress; | ... | ... |
-
Please register or login to post a comment