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.
This commit is contained in:
Miron Cuperman 2012-03-06 12:49:45 -08:00
parent 789798bae0
commit bb97da6a5a
3 changed files with 27 additions and 19 deletions

View File

@ -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;

View File

@ -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.");

View File

@ -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.