-
Notifications
You must be signed in to change notification settings - Fork 10
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
multistream/negotiated: Propagate poll_close on unreceived protocol messages #62
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
// However, for the Litep2p the negotation must conclude before closing the | ||
// lazy negotation of protocol. We'll wait for the close until the | ||
// server has produced a message, in this test that means forever. |
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'm curious why it was implemented like this in the first place. Aren't we in a Chesterton's fence situation here?
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.
Yep indeed, I wonder if this affects something in substrate. Proly would be good to have it running in versi for a while. I'll wait a bit for that and group this PR with the clippy PR (and any others) for testing
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.
cc @altonen would love to get your feedback on this if you remember why the change was needed 🙏
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 honestly don't even remember touching the multistream-select
code. Did you check the behavior in this test against the latest multistream-select
that libp2p uses or some other version? I wonder if it could be related to this: paritytech/substrate#14691
This PR ensures that lazily negotiated protocols can be closed before the specific protocol message has been received.
Closes: #60
cc @paritytech/networking