Make sure _connectedPeer exists before checking it

throughnothing commented 9 years ago

I'm not entirely sure this is the right solution, but I've gotten this error working on my SPV client.

I set a timeout to peer.disconnect() a peer if I don't receive a verack message within 3 seconds, to weed out slow peers from my pool (not sure if that's a good approach or not), but when I call the peer.disconnect() on the timeout, it seems to bubble up to this. My change alleviates that problem, but not sure it's the correct fix or not as I haven't traced through what exactly is going on yet.

/Users/throughnothing/Projects/bitcoin/bitcore-spv/node_modules/bitcore-p2p/lib/pool.js:171
  if (this._connectedPeers[addr.hash].status !== Peer.STATUS.DISCONNECTED) {
                                     ^
TypeError: Cannot read property 'status' of undefined
    at Pool._removeConnectedPeer (/Users/throughnothing/Projects/bitcoin/bitcore-spv/node_modules/bitcore-p2p/lib/pool.js:171:38)
    at Pool.peerDisconnectEvent (/Users/throughnothing/Projects/bitcoin/bitcore-spv/node_modules/bitcore-p2p/lib/pool.js:93:10)
    at Pool.emit (events.js:129:20)
    at Peer.peerDisconnect (/Users/throughnothing/Projects/bitcoin/bitcore-spv/node_modules/bitcore-p2p/lib/pool.js:191:12)
    at Peer.emit (events.js:104:17)
    at Peer.disconnect (/Users/throughnothing/Projects/bitcoin/bitcore-spv/node_modules/bitcore-p2p/lib/peer.js:148:8)
    at null._onTimeout (/Users/throughnothing/Projects/bitcoin/bitcore-spv/lib/pool.js:94:10)
    at Timer.listOnTimeout (timers.js:110:15)

coveralls commented 9 years ago

Changes Unknown when pulling 389ad2c355704d663e1542275a6a35addc26a35a on throughnothing:check-connectedPeer into \ on bitpay:master**.

braydonf commented 9 years ago

A few notes:

pool.disconnect() goes through the keys of_connectedPeers: https://github.com/bitpay/bitcore-p2p/blob/master/lib/pool.js#L134 (if it doesn't exist it wouldn't be called).
calling peer.disconnet() will remove it from the _connectedPeers: https://github.com/bitpay/bitcore-p2p/blob/master/lib/pool.js#L91 (no other action needed).
the only place that _removeConnectedPeer is called is when a peer disconnects, so if it's not in the _connectedPeers object there might be another problem (e.g. a peer is not in _connectedPeers when it should be).

throughnothing commented 9 years ago

@braydonf that was my concern. This 'fix' in this PR is probably just masking the real issue.

braydonf commented 9 years ago

Also I believe a peer will emit "disconnect" if there isn't "ready" event, perhaps there is a way to may this happen more quickly using a timeout?

Edit: Looks like disconnect happens on socket end or if the buffer is above the max: https://github.com/bitpay/bitcore-p2p/blob/master/lib/peer.js#L120

braydonf commented 9 years ago

We found an issue where disconnect was called twice and was likely the root of the issue, and is now fixed: https://github.com/bitpay/bitcore-p2p/commit/f8578ff114b2911d9b1dfe95b857d2d964928200

throughnothing commented 9 years ago

Thank you!

bitpay / bitcore-p2p

Make sure _connectedPeer exists before checking it #43