Fix/rc active peers #1120
Fix/rc active peers #1120
Conversation
It occurred quite rare but turned into a ghost peer connection which is actually dead, but triggered in tx and blocks propagation threads, thus messages were queued in memory with no chance for release. ChannelManager#newPeers is a type of CopyOnWriteArrayList and when notifyDisconnect was called peer had been simply removed from a copy of newPeers list while processNewPeers execution added it into activePeers at the same time.
| @@ -153,7 +153,7 @@ public void connect(Node node) { | |||
| return ids; | |||
| } | |||
|
|
|||
| private void processNewPeers() { | |||
| private synchronized void processNewPeers() { | |||
There was a problem hiding this comment.
Such things like channel.disconnect() under lock make me nervous :(
I.e. we are calling method which can potentially traverse many components. This is very deadlock prone approach imho
| @@ -344,7 +344,7 @@ public void add(Channel peer) { | |||
| newPeers.add(peer); | |||
| } | |||
|
|
|||
| public void notifyDisconnect(Channel channel) { | |||
| public synchronized void notifyDisconnect(Channel channel) { | |||
| logger.debug("Peer {}: notifies about disconnect", channel); | |||
| channel.onDisconnect(); | |||
There was a problem hiding this comment.
The same here. There is no need to call those notify methods under the lock. They can potentially have deadlock side effects
|
IMHO synchronizing such central manager class should be done very carefully. Don't like outer calls under locks in the original fix. They are potentially deadlock prone. Even though the current implementation may not deadlock minor harmless change on the other side may cause the problem. The only thing we need to assure here is that on |
|
|
||
| processed.add(peer); | ||
| if (addCnt > 0) { |
There was a problem hiding this comment.
addCnt is not used anymore in this code
No description provided.