From 9bfcb80516c76b9c78fa0cc345970a92e15b743b Mon Sep 17 00:00:00 2001 From: Andreas Schildbach Date: Thu, 16 Oct 2014 12:09:10 +0200 Subject: [PATCH] PeerGroup improvements: 1) Don't hold the PeerGroup lock across DNS discovery, otherwise the API is high latency in this period of startup. Fixes issue in Lighthouse where the UI would not appear until DNS resolution had completed. 2) Don't backoff peers that failed due to a first-time connection error. 3) If an IPv6 peer fails to connect due to a NoRouteToHostException, don't try any more IPv6 peers in future. --- .../java/org/bitcoinj/core/PeerGroup.java | 58 ++++++++++++------- 1 file changed, 36 insertions(+), 22 deletions(-) diff --git a/core/src/main/java/org/bitcoinj/core/PeerGroup.java b/core/src/main/java/org/bitcoinj/core/PeerGroup.java index 37631332..bc2cd07d 100644 --- a/core/src/main/java/org/bitcoinj/core/PeerGroup.java +++ b/core/src/main/java/org/bitcoinj/core/PeerGroup.java @@ -32,7 +32,7 @@ import org.bitcoinj.utils.Threading; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; -import com.google.common.collect.Sets; +import com.google.common.collect.Lists; import com.google.common.net.InetAddresses; import com.google.common.primitives.Ints; import com.google.common.primitives.Longs; @@ -44,8 +44,10 @@ import org.slf4j.LoggerFactory; import javax.annotation.Nullable; import java.io.IOException; +import java.net.Inet6Address; import java.net.InetAddress; import java.net.InetSocketAddress; +import java.net.NoRouteToHostException; import java.net.Socket; import java.util.*; import java.util.concurrent.*; @@ -123,6 +125,7 @@ public class PeerGroup extends AbstractExecutionThreadService implements Transac private long pingIntervalMsec = DEFAULT_PING_INTERVAL_MSEC; @GuardedBy("lock") private boolean useLocalhostPeerWhenPossible = true; + @GuardedBy("lock") private boolean ipv6Unreachable = false; private final NetworkParameters params; private final AbstractBlockChain chain; @@ -255,7 +258,7 @@ public class PeerGroup extends AbstractExecutionThreadService implements Transac @Override public void onPeerDisconnected(Peer peer, int peerCount) { // The channel will be automatically removed from channels. - handlePeerDeath(peer); + handlePeerDeath(peer, null); } } @@ -658,26 +661,30 @@ public class PeerGroup extends AbstractExecutionThreadService implements Transac } protected void discoverPeers() throws PeerDiscoveryException { + checkState(lock.isHeldByCurrentThread()); if (peerDiscoverers.isEmpty()) throw new PeerDiscoveryException("No peer discoverers registered"); long start = System.currentTimeMillis(); - Set addressSet = Sets.newHashSet(); + final List addressList = Lists.newLinkedList(); for (PeerDiscovery peerDiscovery : peerDiscoverers) { InetSocketAddress[] addresses; - addresses = peerDiscovery.getPeers(5, TimeUnit.SECONDS); - for (InetSocketAddress address : addresses) addressSet.add(new PeerAddress(address)); - if (addressSet.size() > 0) break; - } - lock.lock(); - try { - for (PeerAddress address : addressSet) { - addInactive(address); - } - } finally { + // Don't hold the peergroup lock across peer discovery as it's likely to be very slow and would make the + // peergroup API high latency. lock.unlock(); + try { + addresses = peerDiscovery.getPeers(5, TimeUnit.SECONDS); + } finally { + lock.lock(); + } + for (InetSocketAddress address : addresses) addressList.add(new PeerAddress(address)); + if (addressList.size() > 0) break; } + for (PeerAddress address : addressList) { + addInactive(address); + } + log.info("Peer discovery took {}msec and returned {} items", - System.currentTimeMillis() - start, addressSet.size()); + System.currentTimeMillis() - start, addressList.size()); } @Override @@ -758,6 +765,7 @@ public class PeerGroup extends AbstractExecutionThreadService implements Transac return; } if (!haveReadyInactivePeer(nowMillis)) { + // Release the lock here because we'll probably do slow things like DNS lookups below, discoverPeers(); groupBackoff.trackSuccess(); nowMillis = Utils.currentTimeMillis(); @@ -766,7 +774,8 @@ public class PeerGroup extends AbstractExecutionThreadService implements Transac log.debug("Peer discovery didn't provide us any more peers, not trying to build new connection."); return; } - addr = inactives.poll(); + while (addr == null || (ipv6Unreachable && addr.getAddr() instanceof Inet6Address)) + addr = inactives.poll(); retryTime = backoffMap.get(addr).getRetryTime(); } finally { // discoverPeers might throw an exception if something goes wrong: we then hit this path with addr == null. @@ -1025,7 +1034,7 @@ public class PeerGroup extends AbstractExecutionThreadService implements Transac channels.openConnection(address.toSocketAddress(), peer); } catch (Exception e) { log.warn("Failed to connect to " + address + ": " + e.getMessage()); - handlePeerDeath(peer); + handlePeerDeath(peer, e); return null; } peer.setSocketTimeout(connectTimeoutMillis); @@ -1275,12 +1284,12 @@ public class PeerGroup extends AbstractExecutionThreadService implements Transac } } - protected void handlePeerDeath(final Peer peer) { + protected void handlePeerDeath(final Peer peer, @Nullable Exception exception) { // Peer deaths can occur during startup if a connect attempt after peer discovery aborts immediately. final State state = state(); if (state != State.RUNNING && state != State.STARTING) return; - int numPeers = 0; + int numPeers; int numConnectedPeers = 0; lock.lock(); try { @@ -1307,10 +1316,15 @@ public class PeerGroup extends AbstractExecutionThreadService implements Transac groupBackoff.trackFailure(); - //TODO: if network failure is suspected, do not backoff peer - backoffMap.get(address).trackFailure(); - // Put back on inactive list - inactives.offer(address); + if (!(exception instanceof NoRouteToHostException)) { + if (address.getAddr() instanceof Inet6Address && !ipv6Unreachable) { + ipv6Unreachable = true; + log.warn("IPv6 peer connect failed due to routing failure, ignoring IPv6 addresses from now on"); + } + backoffMap.get(address).trackFailure(); + // Put back on inactive list + inactives.offer(address); + } if (numPeers < getMaxConnections()) { triggerConnections();