rework peer tracking logic to handle multiple connections #132
Conversation
ghost
assigned 
|
cc @cheatfate |
vyzo
force-pushed
the
fix/peer-tracking
branch
from
234bef0 to
2621f89
Compare
vyzo
force-pushed
the
fix/peer-tracking
branch
from
be6d7e4 to
fc7795c
Compare
|
I believe this is still going to be pretty racy: Hello Packet An old Fixes:
Just 2 may be sufficient but I'm not sure. This will also get rid of that sleep... |
|
2 should be sufficient I think, will update. |
| if p.host.Network().Connectedness(pid) == inet.Connected { | ||
| // still connected, must be a duplicate connection being closed. | ||
| // we respawn the writer as we need to ensure there is a stream active | ||
| log.Warning("peer declared dead but still connected; respawning writer: ", pid) |
There was a problem hiding this comment.
There's a chance we'll hit this frequently the connection may close before we realize we're disconnected. It may not be ab issue but we should watch for this.
| p.rt.AddPeer(pid, s.Protocol()) | ||
|
|
||
| case pid := <-p.newPeerError: | ||
| delete(p.peers, pid) |
There was a problem hiding this comment.
There's a race here where:
- We have an old zombie connection that doesn't know it's dead yet.
- We get a new connection.
With the current logic, we may still try to open a stream using the old connection. We'll then never try to create one with the new connection.
Unfortunately, this is a pretty common situation when a connection dies and we replace it immediately.
Maybe we should drop this channel and send a notification along peerDead instead? If we still think we're connected, we'll continue to retry until we realize we're actually disconnected.
There was a problem hiding this comment.
This is only used when the stream failed to open; we can't pass it through the peerDead channel because the peer may simply not support the pubsub protocols.
There was a problem hiding this comment.
The problem here is if the stream fails to open because the connection was closed (if it was a duplicate connection).
Is there a way to discriminate whether the failure was an unsupported protocol?
In this case we can notify on peerError for unsupported protocol and peerDead for connection closed errors.
There was a problem hiding this comment.
named error in go-multistream.ErrNotSupported can be used for this per discussion on irc.
|
(why does connection state logic have to be this damn hard...) |
Multiple connections are simply not handled at all by the current code; see #130.
This is an attempt to rework the peer tracking logic so that multiple connections are handled gracefully.