-
Notifications
You must be signed in to change notification settings - Fork 949
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
refactor(relay): move stream-handling away from {In,Out}boundUpgrade
#4275
Conversation
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.
The relay protocol is quite complex but you seem to be making great progress which is impressive :)
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.
Amazing work so far!
I've done a review of the server's handler and the involved protocol. Let me know if you have any questions! Most of the style-suggestions likely apply to the other handler and protocol as well :)
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.
Great progress! I've made some more comments to steer you into the right direction.
Thanks a lot for your help with this PR, it is much appreciated!
Yes! We have a public slack channel. You can join here: https://join.slack.com/share/enQtNTg5OTA0MzIwNjk0NC1iZGQ0YzIxYzdiMTUxNjZhNDJiZjJiMWQyNDM1N2U1MWY3YmY4NWI2ZjgyZWY0NzBmMTBiNTNkMzQ4OWFhNzJk
I think it exists mostly for consistency across the 2 different protocols and inbound / outbound variants. I'd suggest to keep it :) |
Hi, @thomaseizinger ! |
Yesterday, I managed to build a first version of an interop test for hole-punching which also uses the relay. See https://github.com/libp2p/rust-libp2p/tree/feat/hole-punching-tests/hole-punching-tests. I've plugged this branch into the tests and they still work! I'd consider that enough confidence that we didn't break anything :) |
It's good news. But, anyway, I want to try relay manually |
Perhaps have a read through the hole-punching tutorial: https://docs.rs/libp2p/0.52.3/libp2p/tutorials/hole_punching/index.html |
@mxinden Can you give this a final review? I've tested it with my in-progress hole-punching setup and it still works :) |
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.
Just some small nit-picks. Otherwise this looks good to me. Thank you for the work!
Co-authored-by: Max Inden <[email protected]>
Previously, a connection would be shut down immediately as soon as its `ConnectionHandler` reports `KeepAlive::No`. As we have gained experience with libp2p, it turned out that this isn't ideal. For one, tests often need to keep connections alive longer than the configured protocols require. Plus, some usecases require connections to be kept alive in general. Both of these needs are currently served by the `keep_alive::Behaviour`. That one does essentially nothing other than statically returning `KeepAlive::Yes` from its `ConnectionHandler`. It makes much more sense to deprecate `keep_alive::Behaviour` and instead allow users to globally configure an `idle_conncetion_timeout` on the `Swarm`. This timeout comes into effect once a `ConnectionHandler` reports `KeepAlive::No`. To start with, this timeout is 0. Together with libp2p#3844, this will allow us to move towards a much more aggressive closing of idle connections, together with a more ergonomic way of opting out of this behaviour. Fixes libp2p#4121. Pull-Request: libp2p#4161.
Thank you so much for the review! |
Everything is addressed, no big blockers were mentioned by @mxinden so if anything else comes up, we can always do it in a follow-up! Let's go! |
Description
Fixes: #4075.
Notes & open questions
Change checklist