Skip to content
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(gossipsub): use send_message for all RpcOut #12

Merged

Conversation

elenaf9
Copy link

@elenaf9 elenaf9 commented Nov 20, 2024

Description

Follow-up on libp2p#5595 (comment).

I've played around a bit with restoring the old Behavior::send_message function.
It is now using a new RpcSender::send_message method, that handles sending of all Rpc messages, instead of having separate methods RpcSender::{publish, subscribe, forward, ...}.
The advantages of this design IMO are:

  1. Smaller diff to the current master branch
  2. less code
  3. Behavior::send_message handles all errors in sending. Before, there was quite a bit of code duplication for a) handling that the target peer was not among the connected peers, and b) handling if a channel is full (logging the error, increasing the counter) c) handling that the channel will never be full for high priority control messages.
  4. single match statement in RpcSender::send_message that decides which rpc message is sent on which channel,

Drawbacks:

  1. RpcSender::send_message returns an Result for all rpc message, even though there are some high priority messages where sending can never fail. Behavior::send_message handles a returned error for these variants with an unreachable, but obviously it would be nicer if that logic would be directly inside the RpcSender.

Notes & open questions

I would expect a types module to not implement any major logic apart from converting and formatting.

Wdyt of moving the RpcSender and RpcReceiver into a separate module behavior/rpc.rs or similar?

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

Copy link
Owner

@jxs jxs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to test the solution Elena, and thanks for other parts of code that were also not elegant.
Looks good to me! Left just a couple comments

protocols/gossipsub/src/behaviour.rs Show resolved Hide resolved
protocols/gossipsub/src/behaviour.rs Outdated Show resolved Hide resolved
Copy link
Owner

@jxs jxs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Elena! Going to merge this and address your remaining comments on libp2p#5595

protocols/gossipsub/src/types.rs Show resolved Hide resolved
protocols/gossipsub/src/behaviour.rs Show resolved Hide resolved
@jxs jxs merged commit b626118 into jxs:impl-gossipsub-backpressure-2 Nov 22, 2024
65 of 66 checks passed
elenaf9 added a commit to elenaf9/rust-libp2p that referenced this pull request Nov 22, 2024
@elenaf9 elenaf9 deleted the gossipsub/sender_send_message branch November 22, 2024 15:06
jxs added a commit that referenced this pull request Nov 22, 2024
fix(gossipsub): readd lines accidently removed with #12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants