refactor(rendezvous): rewrite using `libp2p-request-response` by dgarus · Pull Request #4051 · libp2p/rust-libp2p

dgarus · 2023-06-08T13:42:25Z

Description

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

dgarus · 2023-06-08T13:46:35Z

@thomaseizinger
Hello, Thomas!
I did the first version of the PR, could you please take a look?
I have a problem with the can_combine_client_and_server test and while I don't catch why.

thomaseizinger

Amazing work! I'll do an in-depth review early next week!

protocols/rendezvous/src/server.rs

thomaseizinger · 2023-06-09T15:09:50Z

Clippy seems to be unhappy :)

dgarus · 2023-06-09T20:34:08Z

Clippy seems to be unhappy :)

Clippy is satisfied now

thomaseizinger

Thank you! This is a really high-quality contribution!

A few minor comments but overall this looks really good :)

protocols/rendezvous/Cargo.toml

protocols/rendezvous/src/codec.rs

protocols/rendezvous/src/client.rs

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

dgarus · 2023-06-13T09:58:50Z

Thank you! This is a really high-quality contribution!

A few minor comments but overall this looks really good :)

Thanks for your high estimation of my contribution, it really motivates me!

dgarus · 2023-06-13T11:22:53Z

@thomaseizinger
I fixed your comments, please, take a look.

thomaseizinger

Thanks! I am looking forward to merging this.

A few more comments.

protocols/rendezvous/src/client.rs

protocols/rendezvous/src/codec.rs

protocols/rendezvous/src/server.rs

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

protocols/rendezvous/src/client.rs

…p' into 3878-rendezvous-based-on-req-resp

dgarus · 2023-06-14T09:33:50Z

@thomaseizinger
Hello!
Thank you so much for the review.

I fixed your comments, please, take a look.

thomaseizinger

Thanks for the follow-up commits. A few more things but this is very close to being ready :)

protocols/rendezvous/src/client.rs

thomaseizinger · 2023-06-14T09:47:14Z

protocols/rendezvous/src/client.rs

+                None
            }
-            _ => unreachable!(),
+            _ => unreachable!("rendezvous clients never receive requests"),


This isn't safe. We cannot statically ensure that we don't receive a request, the remote could send us any message.

In follow-up work, we could redesign this to have two enums: one for requests and one for responses. Then our codec could ensure that and inbound message on the client is always a response.

protocols/rendezvous/src/codec.rs

protocols/rendezvous/src/server.rs

dgarus · 2023-06-14T13:43:03Z

@thomaseizinger

I removed RendezvousCodec and implemented Decoder/Encoder for Codec.
Looks good to me )) Could you please review?

thomaseizinger

Nicely done, just one final comment!

protocols/rendezvous/src/server.rs

thomaseizinger · 2023-06-14T15:36:32Z

I removed RendezvousCodec and implemented Decoder/Encoder for Codec.

Good idea!!

thomaseizinger

Thanks a lot for doing this refactoring!

It is one of the main remaining blockers for #3268 which is an improvement I am very much looking forward to!

thomaseizinger · 2023-06-14T17:38:55Z

We didn't have to touch a single line in the tests! A picture-perfect refactoring!

dgarus · 2023-06-14T17:46:45Z

Thanks a lot for doing this refactoring!

It is one of the main remaining blockers for #3268 which is an improvement I am very much looking forward to!

Thank you a lot for the review!

dgarus added 3 commits May 31, 2023 16:54

response_request::Codec implementation

9c694dc

Rendezvous based on request_response behaviour

9da802f

Merge branch 'master' into 3878-rendezvous-based-on-req-resp

8c80b08

Merge branch 'master' into 3878-rendezvous-based-on-req-resp

7fdfe43

thomaseizinger reviewed Jun 9, 2023

View reviewed changes

protocols/rendezvous/src/server.rs Outdated Show resolved Hide resolved

Fix test failure

2a794e0

dgarus marked this pull request as ready for review June 9, 2023 12:38

dgarus added 3 commits June 9, 2023 23:18

Fixing clippy fail

386b503

Fixing clippy fail

59663ef

Fixing clippy fail

1e738f8

thomaseizinger reviewed Jun 10, 2023

View reviewed changes

thomaseizinger mentioned this pull request Jun 10, 2023

core: remove upgrade::transfer module #4011

Closed

dgarus and others added 3 commits June 12, 2023 22:38

Merge branch 'master' into 3878-rendezvous-based-on-req-resp

ed26e82

Update protocols/rendezvous/Cargo.toml

c0751f9

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

Update protocols/rendezvous/src/codec.rs

4ce78c5

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

thomaseizinger changed the title ~~feat: Rendezvous based on libp2p-request-response protocol~~ refactor(rendezvous): rewrite using libp2p-request-response Jun 13, 2023

dgarus added 2 commits June 13, 2023 12:51

Fixed review comments

111b640

Fixed review comments

2990635

dgarus added 6 commits June 13, 2023 13:25

Fixed review comments

3d7c5ee

Fixed review comments

a8fa740

Merge branch 'master' into 3878-rendezvous-based-on-req-resp

1656e29

Merge branch 'master' into 3878-rendezvous-based-on-req-resp

a33519a

Fixed review comments

20dd3a3

Merge branch 'master' into 3878-rendezvous-based-on-req-resp

fe3d347

Merge branch 'master' into 3878-rendezvous-based-on-req-resp

7deff18

Merge branch 'master' into 3878-rendezvous-based-on-req-resp

677fafc

thomaseizinger reviewed Jun 13, 2023

View reviewed changes

dgarus and others added 3 commits June 13, 2023 17:03

Update protocols/rendezvous/src/client.rs

5d0cfdc

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

Use combinator for handling EOF

1d2716b

Handle OutboundFailure for register and discover requests

75f9e0f

dgarus commented Jun 14, 2023

View reviewed changes

protocols/rendezvous/src/client.rs Show resolved Hide resolved

dgarus added 4 commits June 14, 2023 11:46

Fixed review comments

1a615c4

Merge branch 'master' into 3878-rendezvous-based-on-req-resp

01965ac

Merge remote-tracking branch 'origin/3878-rendezvous-based-on-req-res…

6b64e22

…p' into 3878-rendezvous-based-on-req-resp

Fixed the server's behaviour fn poll()

9a6e0dd

Fixed fmt fail

15921f6

thomaseizinger reviewed Jun 14, 2023

View reviewed changes

dgarus added 6 commits June 14, 2023 14:32

Refactoring

eb99764

Refactoring server

b71b961

Removed the recursion

dfeb11e

fn poll is wrapped into the loop

0a1e48a

Got rid of RendezvousCodec

58e75f6

fixed fmt

ba058d5

thomaseizinger reviewed Jun 14, 2023

View reviewed changes

protocols/rendezvous/src/server.rs Outdated Show resolved Hide resolved

dgarus added 2 commits June 14, 2023 20:16

Merge branch 'master' into 3878-rendezvous-based-on-req-resp

216cf1c

Fixed review comment

d4c65d4

thomaseizinger approved these changes Jun 14, 2023

View reviewed changes

thomaseizinger added the send-it label Jun 14, 2023

mergify bot merged commit b8ceecc into libp2p:master Jun 14, 2023

dgarus deleted the 3878-rendezvous-based-on-req-resp branch June 15, 2023 11:27

Conversation

dgarus commented Jun 8, 2023 • edited by thomaseizinger Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes & open questions

Change checklist

Uh oh!

dgarus commented Jun 8, 2023

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thomaseizinger commented Jun 9, 2023

Uh oh!

dgarus commented Jun 9, 2023

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dgarus commented Jun 13, 2023

Uh oh!

dgarus commented Jun 13, 2023

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dgarus commented Jun 14, 2023

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thomaseizinger Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

thomaseizinger Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dgarus commented Jun 14, 2023

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thomaseizinger commented Jun 14, 2023

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

thomaseizinger commented Jun 14, 2023

Uh oh!

dgarus commented Jun 14, 2023

Uh oh!

Reviewers

Assignees

dgarus commented Jun 8, 2023 •

edited by thomaseizinger

Loading