Upgrade libp2p-kad to stable futures #1254
Conversation
| @@ -485,7 +484,10 @@ where | |||
| _ => false, | |||
| }); | |||
| if let Some(pos) = pos { | |||
| let _ = self.substreams.remove(pos).try_close(); | |||
| // TODO: we don't properly close down the substream | |||
There was a problem hiding this comment.
Note that this is consistent with the current behaviour, but I realized that if the substream wasn't ready to be closed, we would just drop it immediately. That's not great.
There was a problem hiding this comment.
That was intentional though, i.e. KademliaHandlerIn::Reset is just supposed to drop (reset) the substream. Trying to see if a close succeeds without blocking was just a "why not?", essentially. Do you see problems with this approach?
There was a problem hiding this comment.
Closing a substream requires sending a message saying "close this substream" on the wire, and the closing process normally does that.
If a substream is dropped without being properly closed, the implementation of StreamMuxer can detect that and send the "close" message on the wire the next time it gets polled, but that's not an ideal way to do and I'm not sure that we're actually doing this.
There was a problem hiding this comment.
I'm ok with the current approach as long as we refactor it later with async/await.
protocols/kad/src/handler.rs
Outdated
| @@ -103,29 +102,29 @@ where | |||
|
|
|||
| impl<TSubstream, TUserData> SubstreamState<TSubstream, TUserData> | |||
| where | |||
| TSubstream: AsyncRead + AsyncWrite, | |||
| TSubstream: AsyncRead + AsyncWrite + Unpin, | |||
| { | |||
| /// Consumes this state and tries to close the substream. | |||
There was a problem hiding this comment.
The comment "consumes this state" no longer matches the implementation (self -> &mut self).
| @@ -485,7 +484,10 @@ where | |||
| _ => false, | |||
| }); | |||
| if let Some(pos) = pos { | |||
| let _ = self.substreams.remove(pos).try_close(); | |||
| // TODO: we don't properly close down the substream | |||
There was a problem hiding this comment.
That was intentional though, i.e. KademliaHandlerIn::Reset is just supposed to drop (reset) the substream. Trying to see if a close succeeds without blocking was just a "why not?", essentially. Do you see problems with this approach?
I didn't check if the tests were working (they're most likely not) because they depend on
libp2p-yamux, which isn't upgraded yet.