From bb97da6a5ab78f58ddc62af27d72e24671a863a9 Mon Sep 17 00:00:00 2001
From: Miron Cuperman <miron@google.com>
Date: Tue, 6 Mar 2012 12:49:45 -0800
Subject: [PATCH] Fix race condition on PeerGroup shutdown.

peers can be null in handlePeerDeath if we are shutting down.
Remove redundant numPeers() - use numConnectedPeers().
Rename getPeers() to getConnectedPeers()

Resolves issue 147.
---
 src/com/google/bitcoin/core/Peer.java         |  2 +
 src/com/google/bitcoin/core/PeerGroup.java    | 38 +++++++++++--------
 .../google/bitcoin/core/PeerGroupTest.java    |  6 +--
 3 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/src/com/google/bitcoin/core/Peer.java b/src/com/google/bitcoin/core/Peer.java
index 9e8b749f..466beb3d 100644
--- a/src/com/google/bitcoin/core/Peer.java
+++ b/src/com/google/bitcoin/core/Peer.java
@@ -688,6 +688,8 @@ public class Peer {
 
     /**
      * Terminates the network connection and stops the message handling loop.
+     * 
+     * <p>This does not wait for the loop to terminate.
      */
     public synchronized void disconnect() {
         running = false;
diff --git a/src/com/google/bitcoin/core/PeerGroup.java b/src/com/google/bitcoin/core/PeerGroup.java
index 6d630f81..06a93c5d 100644
--- a/src/com/google/bitcoin/core/PeerGroup.java
+++ b/src/com/google/bitcoin/core/PeerGroup.java
@@ -271,22 +271,14 @@ public class PeerGroup {
     
     /**
      * Returns a newly allocated list containing the currently connected peers. If all you care about is the count,
-     * use numPeers().
+     * use numConnectedPeers().
      */
-    public synchronized List<Peer> getPeers() {
+    public synchronized List<Peer> getConnectedPeers() {
         ArrayList<Peer> result = new ArrayList<Peer>(peers.size());
         result.addAll(peers);
         return result;
     }
 
-    /**
-     * Returns the number of currently connected peers. To be informed when this count changes, register a 
-     * {@link PeerEventListener} and use the onPeerConnected/onPeerDisconnected methods.
-     */
-    public synchronized int numPeers() {
-        return peers.size();
-    }
-
     /**
      * Add an address to the list of potential peers to connect to
      */
@@ -312,10 +304,12 @@ public class PeerGroup {
     }
 
     /**
-     * Stop this PeerGroup.<p>
+     * Stop this PeerGroup.
      *
-     * The peer group will be asynchronously shut down.  After it is shut down all peers will be disconnected and no
-     * threads will be running.
+     * <p>The peer group will be asynchronously shut down.  Some time after it is shut down all peers
+     * will be disconnected and no threads will be running.
+     * 
+     * <p>It is an error to call any other method on PeerGroup after calling this one.
      */
     public synchronized void stop() {
         if (running) {
@@ -375,9 +369,14 @@ public class PeerGroup {
         removeEventListener(wallet.getPeerEventListener());
     }
 
-    /** Returns how many remote nodes this peer group is connected to. */
-    public int numConnectedPeers() {
-        return peers.size();
+    /**
+     * Returns the number of currently connected peers. To be informed when this count changes, register a 
+     * {@link PeerEventListener} and use the onPeerConnected/onPeerDisconnected methods.
+     */
+    public synchronized int numConnectedPeers() {
+        synchronized (peers) {
+            return peers.size();
+        }
     }
 
     public synchronized boolean isRunning() {
@@ -434,6 +433,9 @@ public class PeerGroup {
                 }
             } catch (InterruptedException ex) {
             }
+            
+            // We were asked to stop.  Reset running flag and disconnect all peers.  Peers could
+            // still linger until their event loop is scheduled.
             synchronized (PeerGroup.this) {
                 running = false;
                 peerPool.shutdown();
@@ -708,6 +710,10 @@ public class PeerGroup {
     }
 
     protected synchronized void handlePeerDeath(final Peer peer) {
+        if (!isRunning()) {
+            log.info("Peer death while shutting down");
+            return;
+        }
         assert !peers.contains(peer);
         if (peer == downloadPeer) {
             log.info("Download peer died. Picking a new one.");
diff --git a/tests/com/google/bitcoin/core/PeerGroupTest.java b/tests/com/google/bitcoin/core/PeerGroupTest.java
index 5fd4c536..178c8435 100644
--- a/tests/com/google/bitcoin/core/PeerGroupTest.java
+++ b/tests/com/google/bitcoin/core/PeerGroupTest.java
@@ -115,8 +115,8 @@ public class PeerGroupTest extends TestWithNetworkConnections {
         peerGroup.addPeer(p2);
         
         // Check the peer accessors.
-        assertEquals(2, peerGroup.numPeers());
-        Set<Peer> tmp = new HashSet<Peer>(peerGroup.getPeers());
+        assertEquals(2, peerGroup.numConnectedPeers());
+        Set<Peer> tmp = new HashSet<Peer>(peerGroup.getConnectedPeers());
         Set<Peer> expectedPeers = new HashSet<Peer>();
         expectedPeers.add(p1);
         expectedPeers.add(p2);
@@ -145,7 +145,7 @@ public class PeerGroupTest extends TestWithNetworkConnections {
         peerGroup.start();
         peerGroup.addPeer(p1);
         peerGroup.addPeer(p2);
-        assertEquals(2, peerGroup.numPeers());
+        assertEquals(2, peerGroup.numConnectedPeers());
 
         // Set up a little block chain. We heard about b1 but not b2 (it is pending download). b3 is solved whilst we
         // are downloading the chain.