Move connection management into networking layer #351
Conversation
| return nil | ||
| case <-s.done: | ||
| return nil | ||
| case <-time.After(s.opts.SendErrorBackoff): |
There was a problem hiding this comment.
I'd avoid using time.After but this isn't too critical.
| } | ||
| state.refs++ | ||
|
|
||
| if state.refs == 1 && state.responsive { |
There was a problem hiding this comment.
I'd consider switching the peer back to "responsive" on connect.
There was a problem hiding this comment.
That is:
if state.refs == 1 || !state.responsive {
state.responsive = true
...
}There was a problem hiding this comment.
We get into the unresponsive state if the remote peer fails to respond to several attempts to dial it, but it's still connected. So we're implicitly saying that we care about responsiveness more than connectivity.
I guess arguably if a peer opens a new connection it can be considered responsive?
There was a problem hiding this comment.
Thinking about this more we probably want to keep it how it is - if for example a peer that doesn't support bitswap dials us, we will dial it when broadcasting to connected peers. If the peer responds with an error indicating protocol not supported, we shouldn't try to dial it again even if it connects to us again.
There was a problem hiding this comment.
Given that we're making 3 attempts, it's probably fine. I'm concerned that it can sometimes take some time to know that a connection is actually dead. In that case, we could try several times, say "peer's dead!", then get the new connection, then see the old connection finally die.
| return s.stream, nil | ||
| } | ||
|
|
||
| // Reset the stream | ||
| func (s *streamMessageSender) Reset() error { | ||
| if s.stream != nil { | ||
| err := s.stream.Reset() | ||
| s.stream = nil | ||
| s.connected = false |
There was a problem hiding this comment.
Why not just set the stream to nil? That will free up the resources as well.
There was a problem hiding this comment.
I believe the crash was caused because we were calling SupportsHave() after a Reset():
func (s *streamMessageSender) SupportsHave() bool {
return s.bsnet.SupportsHave(s.stream.Protocol())
}
There was a problem hiding this comment.
Ah, I see. We shouldn't even construct a streamMessageSender till we have the stream.
|
Move connection management into networking layer This commit was moved from ipfs/go-bitswap@9d9719e
Part of the fix for #347