Timeout all Identify stream reads #1032
Conversation
p2p/protocol/identify/id_push.go
Outdated
| ids.handleIdentifyResponse(s) | ||
| } | ||
| } |
p2p/protocol/identify/id_push.go
Outdated
| @@ -13,5 +14,7 @@ const IDPush = "/ipfs/id/push/1.0.0" | |||
|
|
|||
| // pushHandler handles incoming identify push streams. The behaviour is identical to the ordinary identify protocol. | |||
| 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
| @@ -15,6 +16,8 @@ const IDDelta = "/p2p/id/delta/1.0.0" | |||
|
|
|||
| // 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.
| @@ -408,7 +414,9 @@ func (ids *IDService) sendIdentifyResp(s network.Stream) { | |||
| log.Debugf("%s sent message to %s %s", ID, c.RemotePeer(), c.RemoteMultiaddr()) | |||
| } | |||
|
|
|||
| 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.
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. |
| @@ -408,7 +414,9 @@ func (ids *IDService) sendIdentifyResp(s network.Stream) { | |||
| log.Debugf("%s sent message to %s %s", ID, c.RemotePeer(), c.RemoteMultiaddr()) | |||
| } | |||
|
|
|||
| 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.