Use sync instead of locks. It's simpler and clearer
Project: http://git-wip-us.apache.org/repos/asf/curator/repo Commit: http://git-wip-us.apache.org/repos/asf/curator/commit/1a16f82b Tree: http://git-wip-us.apache.org/repos/asf/curator/tree/1a16f82b Diff: http://git-wip-us.apache.org/repos/asf/curator/diff/1a16f82b Branch: refs/heads/master Commit: 1a16f82baae0360a2823db2cc811fbeb3d6e1392 Parents: f8b67dc Author: randgalt <randg...@apache.org> Authored: Mon Apr 27 14:51:46 2015 -0500 Committer: randgalt <randg...@apache.org> Committed: Mon Apr 27 14:51:46 2015 -0500 ---------------------------------------------------------------------- .../curator/x/discovery/details/Holder.java | 106 ++++--------------- .../discovery/details/ServiceDiscoveryImpl.java | 24 +---- 2 files changed, 26 insertions(+), 104 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/curator/blob/1a16f82b/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/Holder.java ---------------------------------------------------------------------- diff --git a/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/Holder.java b/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/Holder.java index cbc6236..d088f8d 100644 --- a/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/Holder.java +++ b/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/Holder.java @@ -2,8 +2,6 @@ package org.apache.curator.x.discovery.details; import org.apache.curator.framework.recipes.cache.NodeCache; import org.apache.curator.x.discovery.ServiceInstance; -import java.util.concurrent.locks.Lock; -import java.util.concurrent.locks.ReentrantLock; class Holder<T> { @@ -18,7 +16,6 @@ class Holder<T> private NodeCache cache; private State state; private long stateChangeMs; - private final ReentrantLock lock = new ReentrantLock(); Holder(ServiceInstance<T> service, NodeCache nodeCache) { @@ -27,110 +24,49 @@ class Holder<T> setState(State.NEW); } - ServiceInstance<T> getService() + synchronized ServiceInstance<T> getService() { - lock.lock(); - try - { - return service; - } - finally - { - lock.unlock(); - } + return service; } - ServiceInstance<T> getServiceIfRegistered() + synchronized ServiceInstance<T> getServiceIfRegistered() { - lock.lock(); - try - { - return (state == State.REGISTERED) ? service : null; - } - finally - { - lock.unlock(); - } + return (state == State.REGISTERED) ? service : null; } - void setService(ServiceInstance<T> service) + synchronized void setService(ServiceInstance<T> service) { - lock.lock(); - try - { - this.service = service; - } - finally - { - lock.unlock(); - } + this.service = service; } - NodeCache getAndClearCache() + synchronized NodeCache getAndClearCache() { - lock.lock(); - try - { - NodeCache localCache = cache; - cache = null; - return localCache; - } - finally - { - lock.unlock(); - } + NodeCache localCache = cache; + cache = null; + return localCache; } - boolean isRegistered() + synchronized boolean isRegistered() { - lock.lock(); - try - { - return state == State.REGISTERED; - } - finally - { - lock.unlock(); - } + return state == State.REGISTERED; } - boolean isLapsedUnregistered(int cleanThresholdMs) + synchronized boolean isLapsedUnregistered(int cleanThresholdMs) { - lock.lock(); - try + if ( state == State.UNREGISTERED ) { - if ( state == State.UNREGISTERED ) + long elapsed = System.currentTimeMillis() - stateChangeMs; + if ( elapsed >= cleanThresholdMs ) { - long elapsed = System.currentTimeMillis() - stateChangeMs; - if ( elapsed >= cleanThresholdMs ) - { - return true; - } + return true; } - return false; - } - finally - { - lock.unlock(); - } - } - - void setState(State state) - { - lock.lock(); - try - { - this.state = state; - stateChangeMs = System.currentTimeMillis(); - } - finally - { - lock.unlock(); } + return false; } - Lock getLock() + synchronized void setState(State state) { - return lock; + this.state = state; + stateChangeMs = System.currentTimeMillis(); } } http://git-wip-us.apache.org/repos/asf/curator/blob/1a16f82b/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 8e3e1f9..a35cd3a 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 @@ -59,6 +59,7 @@ import java.util.concurrent.atomic.AtomicLong; /** * A mechanism to register and query service instances using ZooKeeper */ +@SuppressWarnings("SynchronizationOnLocalVariableOrMethodParameter") public class ServiceDiscoveryImpl<T> implements ServiceDiscovery<T> { private final Logger log = LoggerFactory.getLogger(getClass()); @@ -181,9 +182,8 @@ public class ServiceDiscoveryImpl<T> implements ServiceDiscovery<T> { clean(); - Holder<T> holder = getOrMakeHolder(service, null); - holder.getLock().lock(); - try + final Holder<T> holder = getOrMakeHolder(service, null); + synchronized(holder) { if ( !holder.isRegistered() ) { @@ -195,10 +195,6 @@ public class ServiceDiscoveryImpl<T> implements ServiceDiscovery<T> String path = pathForInstance(service.getName(), service.getId()); client.setData().forPath(path, bytes); } - finally - { - holder.getLock().unlock(); - } } @VisibleForTesting @@ -459,18 +455,13 @@ public class ServiceDiscoveryImpl<T> implements ServiceDiscovery<T> { for ( final Holder<T> holder : services.values() ) { - holder.getLock().lock(); - try + synchronized(holder) { if ( holder.isRegistered() ) { internalRegisterService(holder.getService()); } } - finally - { - holder.getLock().unlock(); - } } } @@ -544,8 +535,7 @@ public class ServiceDiscoveryImpl<T> implements ServiceDiscovery<T> { if ( holder != null ) { - holder.getLock().lock(); - try + synchronized(holder) { holder.setState(Holder.State.UNREGISTERED); NodeCache cache = holder.getAndClearCache(); @@ -565,10 +555,6 @@ public class ServiceDiscoveryImpl<T> implements ServiceDiscovery<T> // ignore } } - finally - { - holder.getLock().unlock(); - } } } }