add a timeout during noise handshake #294
Conversation
libp2p/protocols/secure/noise.nim
Outdated
| @@ -265,14 +265,14 @@ template read_s: untyped = | |||
|
|
|||
| proc receiveHSMessage(sconn: Connection): Future[seq[byte]] {.async.} = | |||
| var besize: array[2, byte] | |||
| await sconn.readExactly(addr besize[0], besize.len) | |||
| await sconn.readExactly(addr besize[0], besize.len).wait(30.seconds) | |||
There was a problem hiding this comment.
Should this be configurable (if not, lets at least make it a constant ;-) )?
There was a problem hiding this comment.
Yup will do before merging, was thinking just constant, were could we plug the runtime parameter hmm ? currently it's a bit hard
There was a problem hiding this comment.
Yeah, I think a constant should be enough, I'm surprised that this isn't in the spec somewhere tho?
There was a problem hiding this comment.
Ah no timeouts in spec.
I'm really adding them arbitrarily and just in the handshake for now.
I guess 30 seconds is fine? Unless someone connects from the moon or mars :)
There was a problem hiding this comment.
Yeah, that should be fine for now. But we have to watchout, timeouts are very tricky, I would not merge it until we have finalization passing, which is also failing due to timeouts right now.
There was a problem hiding this comment.
hah indeed, I mostly added it cos the audit issue inspired me to consider this a issue ( a < expected size packet would stuck a connection )
There was a problem hiding this comment.
@dryajov do you think we can add those? (I will make it into a constant btw and raise to 60 seconds)
Or wanna wait until we figure how to test nbc+libp2p?
There was a problem hiding this comment.
Lets merge #295 before anything else, I think I can do that in the next hour or so, once CI is finished.
There was a problem hiding this comment.
we can move this forward now, @sinkingsugar
No description provided.