Repository: curator Updated Branches: refs/heads/CURATOR-131 [created] 5e1f9416e
CURATOR-131 use iterator.remove instead of foreach When iterating over a collection, if we try to remove elements that can lead to undefined behaviour. We should use an iterator and its remove method to do this safely. Project: http://git-wip-us.apache.org/repos/asf/curator/repo Commit: http://git-wip-us.apache.org/repos/asf/curator/commit/e38d1ced Tree: http://git-wip-us.apache.org/repos/asf/curator/tree/e38d1ced Diff: http://git-wip-us.apache.org/repos/asf/curator/diff/e38d1ced Branch: refs/heads/CURATOR-131 Commit: e38d1ceda492e097ad9c94bb854838f2b1f7caa9 Parents: f5767c8 Author: Mike Drob <[email protected]> Authored: Thu Jul 31 16:24:40 2014 -0500 Committer: Mike Drob <[email protected]> Committed: Thu Jul 31 16:24:40 2014 -0500 ---------------------------------------------------------------------- .../discovery/details/DownInstanceManager.java | 11 ++++++--- .../discovery/details/ServiceDiscoveryImpl.java | 26 +++++++++++++++++--- 2 files changed, 31 insertions(+), 6 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/curator/blob/e38d1ced/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/DownInstanceManager.java ---------------------------------------------------------------------- diff --git a/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/DownInstanceManager.java b/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/DownInstanceManager.java index cda0968..5b89566 100644 --- a/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/DownInstanceManager.java +++ b/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/DownInstanceManager.java @@ -19,10 +19,13 @@ package org.apache.curator.x.discovery.details; import com.google.common.collect.Maps; + import org.apache.curator.x.discovery.DownInstancePolicy; import org.apache.curator.x.discovery.InstanceFilter; import org.apache.curator.x.discovery.ServiceInstance; -import java.util.Map; + +import java.util.Iterator; +import java.util.Map.Entry; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; @@ -77,12 +80,14 @@ class DownInstanceManager<T> implements InstanceFilter<T> return; } - for ( Map.Entry<ServiceInstance<?>, Status> entry : statuses.entrySet() ) + Iterator<Entry<ServiceInstance<?>, Status>> it = statuses.entrySet().iterator(); + while ( it.hasNext() ) { + Entry<ServiceInstance<?>, Status> entry = it.next(); long elapsedMs = System.currentTimeMillis() - entry.getValue().startMs; if ( elapsedMs >= downInstancePolicy.getTimeoutMs() ) { - statuses.remove(entry.getKey()); + it.remove(); } } } http://git-wip-us.apache.org/repos/asf/curator/blob/e38d1ced/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceDiscoveryImpl.java ---------------------------------------------------------------------- diff --git a/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceDiscoveryImpl.java b/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceDiscoveryImpl.java index 3924b40..ad6ce89 100644 --- a/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceDiscoveryImpl.java +++ b/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceDiscoveryImpl.java @@ -24,6 +24,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.common.collect.Sets; + import org.apache.curator.utils.CloseableUtils; import org.apache.curator.framework.CuratorFramework; import org.apache.curator.framework.state.ConnectionState; @@ -43,9 +44,11 @@ import org.apache.zookeeper.KeeperException; import org.apache.zookeeper.Watcher; import org.slf4j.Logger; import org.slf4j.LoggerFactory; + import java.io.IOException; import java.io.UnsupportedEncodingException; import java.util.Collection; +import java.util.Iterator; import java.util.List; import java.util.Map; @@ -122,18 +125,35 @@ public class ServiceDiscoveryImpl<T> implements ServiceDiscovery<T> CloseableUtils.closeQuietly(provider); } - for ( ServiceInstance<T> service : services.values() ) + Iterator<ServiceInstance<T>> it = services.values().iterator(); + while ( it.hasNext() ) { + // Should not use unregisterService because of potential ConcurrentModificationException + // so we in-line the bulk of the method here + ServiceInstance<T> service = it.next(); + String path = pathForInstance(service.getName(), service.getId()); + boolean doRemove = true; + try { - unregisterService(service); + client.delete().forPath(path); + } + catch ( KeeperException.NoNodeException ignore ) + { + // ignore } catch ( Exception e ) { + doRemove = false; log.error("Could not unregister instance: " + service.getName(), e); } + + if ( doRemove ) + { + it.remove(); + } } - + client.getConnectionStateListenable().removeListener(connectionStateListener); }
