Timeout all Identify stream reads #1032
Conversation
p2p/protocol/identify/id_push.go
Outdated
|
|
||
| ids.handleIdentifyResponse(s) | ||
| } | ||
| } |
| func (ids *IDService) pushHandler(s network.Stream) { | ||
| s.SetReadDeadline(time.Now().Add(StreamReadTimeout)) | ||
|
|
||
| ids.handleIdentifyResponse(s) |
There was a problem hiding this comment.
Don't we already set a read deadline in this function?
There was a problem hiding this comment.
Have fixed this.
p2p/protocol/identify/id_delta.go
Outdated
|
|
||
| // deltaHandler handles incoming delta updates from peers. | ||
| func (ids *IDService) deltaHandler(s network.Stream) { | ||
| s.SetReadDeadline(time.Now().Add(StreamReadTimeout)) |
There was a problem hiding this comment.
I'd prefer to explicitly ignore the error.
There was a problem hiding this comment.
This is done.
| } | ||
|
|
||
| func (ids *IDService) handleIdentifyResponse(s network.Stream) { | ||
| func (ids *IDService) handleIdentifyResponse(s network.Stream) error { |
There was a problem hiding this comment.
why are we returning an error now? There isn't much we can do about it and we appear to always ignore it.
There was a problem hiding this comment.
This is to ensure that we publish an EvtPeerIdentificationCompleted only if the stream read was successful. If the stream read fails, say because of a timeout, we want to publish an EvtPeerIdentificationFailed event.
There was a problem hiding this comment.
Ah, I see. We should be returning an error from consumeMessage in that case.
p2p/protocol/identify/id.go
Outdated
| } | ||
|
|
||
| func (ids *IDService) sendIdentifyResp(s network.Stream) { | ||
| s.SetDeadline(time.Now().Add(StreamReadTimeout)) |
There was a problem hiding this comment.
nit: using the read timeout here is a bit strange, this mostly applies to writes.
There was a problem hiding this comment.
I agree, had put in just in case. Have removed it.
marten-seemann
left a comment
marten-seemann
left a comment
There was a problem hiding this comment.
Depending on which PR gets merged first (this one or #1033), we might need to set the context for OpenStream here.
|
@Stebalien Have pushed the changes. |
| } | ||
|
|
||
| func (ids *IDService) handleIdentifyResponse(s network.Stream) { | ||
| func (ids *IDService) handleIdentifyResponse(s network.Stream) error { |
There was a problem hiding this comment.
Ah, I see. We should be returning an error from consumeMessage in that case.
No description provided.