kad/executor: Add timeout for writting frames #277
Conversation
Signed-off-by: Alexandru Vasile <[email protected]>
| @@ -91,16 +93,24 @@ impl QueryExecutor { | |||
| /// Send message to remote peer. | |||
| pub fn send_message(&mut self, peer: PeerId, message: Bytes, mut substream: Substream) { | |||
| self.futures.push(Box::pin(async move { | |||
| match substream.send_framed(message).await { | |||
| Ok(_) => QueryContext { | |||
| match tokio::time::timeout(WRITE_TIMEOUT, substream.send_framed(message)).await { | |||
There was a problem hiding this comment.
Not sure if you already considered this, but a timeout is typically not needed for write operations, because send_framed(message)).await will return an error if ACK is not received for sent data on the stream level.
On the other hand, the timeout can complicate sending bigger payloads on slow connections, so I would be extra careful when adding it.
There was a problem hiding this comment.
Thinking about it again, it should be safe to add a 15-sec timeout for writing if we already have a 15-sec timeout for reading 🤔
There was a problem hiding this comment.
I think I missed that ACK on the stream level implementation 🤔 Yep, I would keep the timeout more as a safety net in case we do anything wrong on the lower level implementations 🙏
This PR adds a
WRITE_TIMEOUTfor sending frames to remote peers.The
WRITE_TIMEOUTis set to 15 seconds to mirror theREAD_TIMEOUT.Closes: #231