-
Notifications
You must be signed in to change notification settings - Fork 804
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP EthPeers - remove if disconnected #4100
Conversation
Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>
As suspected, this has caused an issue with callbacks
|
However, it does seem to have eliminated the "keeping disconnected peers" scenario
|
// if duplicate inbound and outbound connections are established at the same time | ||
if (peer.isDisconnected()) { | ||
connections.remove(peerConnection); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the core change here. the rest is logging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a quick fix, but it would be better to sort out the underlying problem wrt how connect / disconnect events are being dispatched. The comment here isn't really accurate, its actually a bug if these events aren't propagated correctly to EthPeers
and it doesn't necessarily have to do with duplicate connections.
On the theory that we're dispatching the disconnect event before the connect event, I haven't looked into this too deeply, but I would probably look at moving connection dispatch logic into PeerConnectionEvents
. From there, you could do something like keep a connectedFuture
, and when dispatchDisconnect is invoked, you could chain that logic to connectedFuture
so that we're ensuring connect and disconnect events are dispatched in order.
@@ -312,9 +313,15 @@ public void processMessage(final Capability cap, final Message message) { | |||
|
|||
@Override | |||
public void handleNewConnection(final PeerConnection connection) { | |||
LOG.trace( | |||
"handleNewConnection {} 6 disconnected? {}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the 6 just a typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you intend to approve a draft PR with test failures?
This is caused by the extra logic in the toString that was added here. The |
Closing this one as it does not seem like a viable approach |
Testing a theory with EthPeer disconnects. From looking at logs, there's a spot in the code where it seems that duplicate connection is detected and disconnected, while the EthPeer is being created and added, so that by the time it is added, it's already disconnected.
This may not be the most elegant solution but I'm running it up and it seems to work but does create an issue with the callbacks - but I think the callback issue might be there anyway.
Documentation
doc-change-required
label to this PR ifupdates are required.
Changelog