mark the peer as dead if the inbound stream closes #39
Conversation
ghost
assigned 
|
Technically, we may still have a connection to the peer but they're still "dead" from the perspective of pubsub (they've closed their pubsub stream to us). |
|
Wait before you merge this. There's an edge case when we receive a second stream from a peer and kill the first. |
1. Don't hang marking a peer as dead if we're shutting down. 2. No need to "drain" the outgoing channel anymore. This may have been necessary to prevent a deadlock where the main loop blocked on sending on sending a message while we waited to tell the main loop that the peer was dead. However, this is no longer an issue (we never block on sending).
|
Actually, there are quite a few existing race conditions around that. I don't think there are any that can cause leaks but many that can cause two peers to fail to connect (e.g., if they both connect to each other at the same time). Really, we should support multiple connections and multiple streams to each peer. However, IMO, that's an issue for another time (#40) and this PR should be good to go (except for the test hangs but I believe that's #22) |
| @@ -74,21 +77,16 @@ func (p *PubSub) handleSendingMessages(ctx context.Context, s inet.Stream, outgo | |||
| if !ok { | |||
| return | |||
| } | |||
| if dead { | |||
There was a problem hiding this comment.
Shouldn't need this anymore (I believe this existed as a hack to prevent some deadlocks but those are no longer possible).
There was a problem hiding this comment.
I believe you are correct. This was because sends on the other end of this outgoing channel could get filled up and block the loops. All those sends are now coupled with default selects (though please double check this for me).
|
I'll do a release once I get libp2p/go-peerstream#13 reviewed once more and merged (so I can bubble both of them up at once). |
fixes #38