refactor(relay): move stream-handling away from `{In,Out}boundUpgrade` #4275

dgarus · 2023-07-31T12:09:40Z

Description

Fixes: #4075.

Notes & open questions

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

thomaseizinger

The relay protocol is quite complex but you seem to be making great progress which is impressive :)

protocols/relay/src/behaviour/handler.rs

thomaseizinger

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 :)

protocols/relay/src/behaviour/handler.rs

thomaseizinger

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!

protocols/relay/src/behaviour/handler.rs

protocols/relay/src/priv_client/handler.rs

protocols/relay/src/behaviour/handler.rs

thomaseizinger · 2023-09-11T09:09:09Z

Do you have a slack or telegram chat?

Yes! We have a public slack channel. You can join here: https://join.slack.com/share/enQtNTg5OTA0MzIwNjk0NC1iZGQ0YzIxYzdiMTUxNjZhNDJiZjJiMWQyNDM1N2U1MWY3YmY4NWI2ZjgyZWY0NzBmMTBiNTNkMzQ4OWFhNzJk

The protocol module outbound_hop looks like artificial to me. So if two functions are moved to the handler module, then we could use ConnectionHandlerEvent as the result and don't have circular dependencies.

I think it exists mostly for consistency across the 2 different protocols and inbound / outbound variants. I'd suggest to keep it :)

dgarus · 2023-09-13T10:18:10Z

Should this testing be my creative or already exists an instruction?

The relay-server example has some basic instructions: https://github.com/libp2p/rust-libp2p/tree/master/examples/relay-server

You can then modify for example the ping example to include a relay client and use the relay's listen address to listen on it. We should be able to have successful pings across the relay.

Hi, @thomaseizinger !
I'm trying to test the relay, but it's not obvious to me how to do.
So I'm in the process.

thomaseizinger · 2023-09-13T10:21:12Z

Should this testing be my creative or already exists an instruction?

The relay-server example has some basic instructions: master/examples/relay-server
You can then modify for example the ping example to include a relay client and use the relay's listen address to listen on it. We should be able to have successful pings across the relay.

Hi, @thomaseizinger ! I'm trying to test the relay, but it's not obvious to me how to do. So I'm in the process.

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 :)

dgarus · 2023-09-13T10:27:45Z

Should this testing be my creative or already exists an instruction?

The relay-server example has some basic instructions: master/examples/relay-server
You can then modify for example the ping example to include a relay client and use the relay's listen address to listen on it. We should be able to have successful pings across the relay.

Hi, @thomaseizinger ! I'm trying to test the relay, but it's not obvious to me how to do. So I'm in the process.

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

thomaseizinger · 2023-09-13T22:55:04Z

Should this testing be my creative or already exists an instruction?

The relay-server example has some basic instructions: master/examples/relay-server
You can then modify for example the ping example to include a relay client and use the relay's listen address to listen on it. We should be able to have successful pings across the relay.

Hi, @thomaseizinger ! I'm trying to test the relay, but it's not obvious to me how to do. So I'm in the process.

Yesterday, I managed to build a first version of an interop test for hole-punching which also uses the relay. See 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

thomaseizinger · 2023-09-17T22:08:07Z

@mxinden Can you give this a final review? I've tested it with my in-progress hole-punching setup and it still works :)

mxinden

Just some small nit-picks. Otherwise this looks good to me. Thank you for the work!

misc/futures-bounded/src/list.rs

misc/futures-bounded/src/map.rs

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.

dgarus · 2023-09-20T08:06:56Z

@thomaseizinger @mxinden

Thank you so much for the review!

thomaseizinger · 2023-09-20T08:10:40Z

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!

Fixes: #4075. Pull-Request: #4275.

Refactoring of stop protocol

79ee923

dgarus marked this pull request as draft July 31, 2023 12:10

Merge branch 'master' into 4075-relay

4db318f

dgarus changed the title ~~Refactoring of stop protocol~~ feat: relay: refactor stream handling away from {In,Out}boundUpgrade Jul 31, 2023

thomaseizinger reviewed Jul 31, 2023

View reviewed changes

protocols/relay/src/behaviour/handler.rs Outdated Show resolved Hide resolved

dgarus added 4 commits August 1, 2023 15:56

fix review comment

1b39aaf

Merge branch 'master' into 4075-relay

aa09aa3

Refactoring of hop protocol

de33b72

Merge branch 'master' into 4075-relay

c5401be

dgarus marked this pull request as ready for review August 2, 2023 13:21

dgarus requested a review from thomaseizinger August 2, 2023 13:22

dgarus added 2 commits August 2, 2023 16:54

Merge branch 'master' into 4075-relay

70bb6d1

Merge branch 'master' into 4075-relay

72b2882

thomaseizinger reviewed Aug 3, 2023

View reviewed changes

protocols/relay/src/behaviour/handler.rs Outdated Show resolved Hide resolved

dgarus added 5 commits August 3, 2023 16:30

fix review comments

0b629da

Merge branch 'master' into 4075-relay

91602d0

fix review comments

dde7f89

Merge branch 'master' into 4075-relay

332e933

fix review comments

55fa4d6

thomaseizinger reviewed Aug 7, 2023

View reviewed changes

dgarus added 2 commits August 7, 2023 12:38

fix review comments

3fb008c

fix review comments

4aac352

thomaseizinger reviewed Aug 7, 2023

View reviewed changes

protocols/relay/src/behaviour/handler.rs Show resolved Hide resolved

dgarus added 6 commits August 7, 2023 13:04

fix review comments

1a1f912

fix review comments

4e699dd

fix rustfmt

1625704

fix review comments

77f4aaa

fix review comments

4be941f

fix review comments

0f79f76

dgarus and others added 5 commits September 11, 2023 16:49

Merge branch 'master' into 4075-relay

8c6d61f

Merge branch 'master' into 4075-relay

0354fc5

Merge branch 'master' into 4075-relay

04c3704

Merge branch 'master' into 4075-relay

ebdcfc0

Fix clippy lint

38d9a28

Allow beta lint

0cb3841

thomaseizinger and others added 2 commits September 14, 2023 09:52

Merge branch 'master' into 4075-relay

5cbe9d7

Merge branch 'master' into 4075-relay

a201778

This was referenced Sep 17, 2023

refactor(identify): use ReadyUpgrade for {In,Out}boundUpgrade #3412

Closed

swarm: Design utility type for tracking pending stream openings #4510

Closed

Merge branch 'master' into 4075-relay

6e63600

mxinden reviewed Sep 19, 2023

View reviewed changes

misc/futures-bounded/src/list.rs Outdated Show resolved Hide resolved

misc/futures-bounded/src/list.rs Outdated Show resolved Hide resolved

misc/futures-bounded/src/list.rs Outdated Show resolved Hide resolved

misc/futures-bounded/src/map.rs Outdated Show resolved Hide resolved

dgarus and others added 4 commits September 19, 2023 20:05

fix review comments

3b8b07d

Update misc/futures-bounded/src/map.rs

d14eb90

Co-authored-by: Max Inden <[email protected]>

Fix formatting

9b3d48c

thomaseizinger requested a review from mxinden September 20, 2023 02:43

Merge branch 'master' into 4075-relay

a872c0c

thomaseizinger added the send-it label Sep 20, 2023

mergify bot merged commit efea490 into libp2p:master Sep 20, 2023
69 checks passed

dgarus deleted the 4075-relay branch September 20, 2023 08:24

thomaseizinger pushed a commit that referenced this pull request Sep 21, 2023

refactor(relay): move stream-handling away from {In,Out}boundUpgrade

d240fb1

Fixes: #4075. Pull-Request: #4275.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(relay): move stream-handling away from `{In,Out}boundUpgrade` #4275

refactor(relay): move stream-handling away from `{In,Out}boundUpgrade` #4275

dgarus commented Jul 31, 2023 •

edited by thomaseizinger

Loading

thomaseizinger left a comment

thomaseizinger left a comment

thomaseizinger left a comment

thomaseizinger commented Sep 11, 2023

dgarus commented Sep 13, 2023 •

edited

Loading

thomaseizinger commented Sep 13, 2023

dgarus commented Sep 13, 2023

thomaseizinger commented Sep 13, 2023

thomaseizinger commented Sep 17, 2023

mxinden left a comment

dgarus commented Sep 20, 2023

thomaseizinger commented Sep 20, 2023

refactor(relay): move stream-handling away from {In,Out}boundUpgrade #4275

refactor(relay): move stream-handling away from {In,Out}boundUpgrade #4275

Conversation

dgarus commented Jul 31, 2023 • edited by thomaseizinger Loading

Description

Notes & open questions

Change checklist

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger commented Sep 11, 2023

dgarus commented Sep 13, 2023 • edited Loading

thomaseizinger commented Sep 13, 2023

dgarus commented Sep 13, 2023

thomaseizinger commented Sep 13, 2023

thomaseizinger commented Sep 17, 2023

mxinden left a comment

Choose a reason for hiding this comment

dgarus commented Sep 20, 2023

thomaseizinger commented Sep 20, 2023

refactor(relay): move stream-handling away from `{In,Out}boundUpgrade` #4275

refactor(relay): move stream-handling away from `{In,Out}boundUpgrade` #4275

dgarus commented Jul 31, 2023 •

edited by thomaseizinger

Loading

dgarus commented Sep 13, 2023 •

edited

Loading