Return-Path: X-Original-To: apmail-curator-commits-archive@minotaur.apache.org Delivered-To: apmail-curator-commits-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 618D611B3A for ; Mon, 11 Aug 2014 06:06:32 +0000 (UTC) Received: (qmail 5550 invoked by uid 500); 11 Aug 2014 06:06:32 -0000 Delivered-To: apmail-curator-commits-archive@curator.apache.org Received: (qmail 5516 invoked by uid 500); 11 Aug 2014 06:06:32 -0000 Mailing-List: contact commits-help@curator.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@curator.apache.org Delivered-To: mailing list commits@curator.apache.org Received: (qmail 5504 invoked by uid 99); 11 Aug 2014 06:06:32 -0000 Received: from tyr.zones.apache.org (HELO tyr.zones.apache.org) (140.211.11.114) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 11 Aug 2014 06:06:32 +0000 Received: by tyr.zones.apache.org (Postfix, from userid 65534) id E05DB9AA3F2; Mon, 11 Aug 2014 06:06:31 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: cammckenzie@apache.org To: commits@curator.apache.org Date: Mon, 11 Aug 2014 06:06:31 -0000 Message-Id: X-Mailer: ASF-Git Admin Mailer Subject: [1/2] git commit: CURATOR-131 use iterator.remove instead of foreach Repository: curator Updated Branches: refs/heads/CURATOR-131 [created] d2c37d0ce 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 Authored: Thu Jul 31 16:24:40 2014 -0500 Committer: Mike Drob 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 implements InstanceFilter return; } - for ( Map.Entry, Status> entry : statuses.entrySet() ) + Iterator, Status>> it = statuses.entrySet().iterator(); + while ( it.hasNext() ) { + Entry, 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 implements ServiceDiscovery CloseableUtils.closeQuietly(provider); } - for ( ServiceInstance service : services.values() ) + Iterator> 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 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); }