Track timeouts per peer #274
Conversation
|
@Stebalien let me know if this looks like the right approach |
|
(I still need to actually review this) |
dirkmc
force-pushed
the
feat/peer-timeout
branch
from
7205ffa
to
ad97437
Compare
dirkmc
force-pushed
the
feat/peer-timeout
branch
from
8e7f0c2
to
38e3452
Compare
dirkmc
force-pushed
the
feat/peer-timeout
branch
from
38e3452
to
805ea89
Compare
| var pm *bspm.PeerManager | ||
| // onPeerResponseTimeout is triggered when a peer fails to respond to any | ||
| // request for a long time | ||
| onPeerResponseTimeout := func(peers []peer.ID) { |
There was a problem hiding this comment.
You can just pass pm.OnTimeout (without the function)
| if rq.active { | ||
| // The queue is in order from earliest to latest, so if we | ||
| // didn't find an expired entry we can stop iterating | ||
| if time.Since(rq.sent) < ptm.peerResponseTimeout { |
There was a problem hiding this comment.
Let's take the time once. Or just use the time from the timer.
| @@ -413,6 +415,9 @@ func (mq *MessageQueue) sendMessage() { | |||
|
|
|||
| mq.simulateDontHaveWithTimeout(message) | |||
|
|
|||
| // Signal that a request was sent | |||
| mq.onRequestSent(mq.p) | |||
There was a problem hiding this comment.
Concerned that doing this after sending the request could cause us to handle this after receiving the response.
| var inOrder []*request | ||
| for { | ||
| select { | ||
| case rq := <-ptm.requestSent: |
There was a problem hiding this comment.
Raised OOB by @dirkmc: we need a single channel to prevent reordering.
| // Keep track of requests sent to a peer, and the order of requests | ||
| requestsSent := make(map[peer.ID]*request) | ||
| var inOrder []*request | ||
| for { |
There was a problem hiding this comment.
Raised OOB in a call: We need to handle cancels. To do that, we're probably going to have to track every request.
|
This approach doesn't really provide enough protection against misbehaving peers for it to be worth it. For example a peer can timeout on half the requests and still remain in the session. Instead we plan to use the existing DONT_HAVE timeout tracking that we apply to older peers, and simply apply that to newer peers also. Closing in favour of #284 |
Fixes ipfs/boxo#125