-
Notifications
You must be signed in to change notification settings - Fork 29
fix: finish reading handshake on lazyConn close #115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -134,9 +134,7 @@ func (l *lazyClientConn[T]) Write(b []byte) (int, error) { | |
| return l.con.Write(b) | ||
| } | ||
|
|
||
| // Close closes the underlying io.ReadWriteCloser | ||
| // | ||
| // This does not flush anything. | ||
| // Close closes the underlying io.ReadWriteCloser after finishing the handshake. | ||
| func (l *lazyClientConn[T]) Close() error { | ||
| // As the client, we flush the handshake on close to cover an | ||
| // interesting edge-case where the server only speaks a single protocol | ||
|
|
@@ -147,6 +145,22 @@ func (l *lazyClientConn[T]) Close() error { | |
| // closed the stream for reading. I mean, we're the initiator so that's | ||
| // strange... but it's still allowed | ||
| _ = l.Flush() | ||
|
|
||
| // Finish reading the handshake before we close the connection/stream. This | ||
| // is necessary so that the other side can finish sending its response to our | ||
| // multistream header before we tell it we are done reading. | ||
| // | ||
| // Example: | ||
| // We open a QUIC stream, write the protocol `/a`, send 1 byte of application | ||
| // data, and immediately close. | ||
| // | ||
| // 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. | ||
|
Comment on lines
+157
to
+160
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should ask js and rust libp2p to fix this on their ends similar to what #87 does.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. First we probably want to define optimistic multistream select so that implementations can reference that: libp2p/specs#643 |
||
| // | ||
| // Note: We currently handle this case in Go(https://github.com/multiformats/go-multistream/pull/87), but rust-libp2p does not. | ||
| l.rhandshakeOnce.Do(l.doReadHandshake) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the read handshake happened in both I am really surprised that Kubo folk never triggered(or realized) this case. Bitswap works there in the exactly same way, without ever calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Btw, the comment on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I guess it did at some point. Go clients handle this correctly with: #87
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think yes because we want to flush on CloseWrite as well. |
||
| return l.con.Close() | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.