fix: finish reading handshake on lazyConn close#115
Conversation
| // Example: | ||
| // We open a QUIC stream, write the protocol `/a`, send 1 byte of application | ||
| // data, and immediately close. |
There was a problem hiding this comment.
Do we hit this case when, for example, we open a stream and then reset it immediately due to an application error?
There was a problem hiding this comment.
It would. But in that case, it's fine to not read the handshake message, as the other side may or may not receive the writes made before calling Reset.
There was a problem hiding this comment.
I was confused when this case happened in practice in our protocol, but I realized that Bitswap opens a streams, writes to it and closes it, potentially never reading handshake data.
There was a problem hiding this comment.
For completeness, servers(the other end of the multistream here) can handle this situation with a fix like: #87 which ignores a reset on writing the multistream header, allowing the user of such a stream to Read all the data that's written.
| // This can result in a single packet that contains the stream data along | ||
| // with a STOP_SENDING frame. The other side may be unable to negotiate | ||
| // multistream select since it can't write to the stream anymore and may | ||
| // drop the stream. |
There was a problem hiding this comment.
We should ask js and rust libp2p to fix this on their ends similar to what #87 does.
There was a problem hiding this comment.
First we probably want to define optimistic multistream select so that implementations can reference that: libp2p/specs#643
| // drop the stream. | ||
| // | ||
| // Note: We currently handle this case in Go, but rust-libp2p does not. | ||
| l.rhandshakeOnce.Do(l.doReadHandshake) |
There was a problem hiding this comment.
So the read handshake happened in both Flush and Write, but it was done asynchronously(loc 172 and 128), and the connection could be closed before readHandshake finishes.... Makes sense
I am really surprised that Kubo folk never triggered(or realized) this case. Bitswap works there in the exactly same way, without ever calling Read, and thus sometimes without reading handshake.
There was a problem hiding this comment.
Btw, the comment on Close states no flushing is happening, but it actually does above.
There was a problem hiding this comment.
Also, as lazy wrapper always Flushes on close, do we actually need another wrapper in basichost https://github.com/libp2p/go-libp2p/blob/7268c98442c34d26a5d87cd72e37c48e1ffe2e6c/p2p/host/basic/basic_host.go#L1151-L1161?
There was a problem hiding this comment.
Thanks for pointing that out. I've been meaning to remove it. I'm not sure why we need to wrap the whole thing at all.
There was a problem hiding this comment.
I am really surprised that Kubo folk never triggered(or realized) this case.
I guess it did at some point. Go clients handle this correctly with: #87
There was a problem hiding this comment.
Also, as lazy wrapper always Flushes on close, do we actually need another wrapper in basichost
I think yes because we want to flush on CloseWrite as well.
Co-authored-by: sukun <sukunrt@gmail.com>
3 participants
fixes libp2p/go-libp2p#3038